Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Allow ReactElement in LabeledValue value #7679

Merged
merged 20 commits into from
Feb 3, 2025

Conversation

sana-malik
Copy link
Contributor

@sana-malik sana-malik commented Jan 28, 2025

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Storybook included as "Custom component" under "LabeledValue"

🧢 Your Project:

Adobe - Developer Home

@sana-malik sana-malik force-pushed the labeledvalue_reactnode branch from 352ffae to 9493cf0 Compare January 28, 2025 20:56
Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! The lint build step is mostly failing due to formatting, so you can run yarn lint to see what to fix for the build to pass

packages/@react-spectrum/labeledvalue/src/LabeledValue.tsx Outdated Show resolved Hide resolved
@sana-malik sana-malik requested a review from reidbarber January 29, 2025 15:10
@sana-malik sana-malik force-pushed the labeledvalue_reactnode branch from 7a00842 to 07ceb2c Compare January 30, 2025 14:55
@sana-malik sana-malik marked this pull request as ready for review January 30, 2025 14:59
@sana-malik sana-malik force-pushed the labeledvalue_reactnode branch from e7b1b5a to 99cb77e Compare January 30, 2025 20:24
type LabeledValueStory = ComponentStoryObj<typeof LabeledValue>;

export default {
const meta: Meta<typeof LabeledValue> = {
title: 'LabeledValue',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding this back in fixed the storybook naming structure

snowystinger
snowystinger previously approved these changes Jan 30, 2025
value={<input />} />
);
} catch (e) {
expect(e.message).toEqual('LabeledValue cannot contain an editable value.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's possible that this isn't throwing, and the test is still passing. We'll want to do the assertion outside of the catch to double check this. I'll try to double check this test when I have a chance. The change looks great, but there may be something we're missing in terms of the test setup. Sorry for the delays!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the assertion outside of the catch!

@sana-malik sana-malik changed the title feat: Allow ReactNode in LabeledValue value feat: Allow ReactElement in LabeledValue value Feb 3, 2025
Copy link
Member

@yihuiliao yihuiliao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! thanks for contribution

@yihuiliao yihuiliao added this pull request to the merge queue Feb 3, 2025
Merged via the queue into adobe:main with commit 2905848 Feb 3, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants