Skip to content

add rest of style props to action menu#2374

Merged
devongovett merged 2 commits into
mainfrom
add-rest-of-style-props-to-actionmenu
Sep 23, 2021
Merged

add rest of style props to action menu#2374
devongovett merged 2 commits into
mainfrom
add-rest-of-style-props-to-actionmenu

Conversation

@snowystinger
Copy link
Copy Markdown
Member

Closes

✅ 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:

🧢 Your Project:

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

@snowystinger snowystinger added the small review Easy to review PR label Sep 23, 2021
reidbarber
reidbarber previously approved these changes Sep 23, 2021
Copy link
Copy Markdown
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.

LGTM

{...buttonProps}
UNSAFE_className={props.UNSAFE_className}
UNSAFE_className={styleProps.className}
UNSAFE_style={styleProps.style}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not pass through other props from props here? This will miss some style props (e.g. hidden), and more if we change the way they work in the future.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

good call, forgot about hidden, would we do it like
{...styleProps as Omit<StyleProps, 'className' | 'style'>}
or would a utility to re-convert to UNSAFE be a good idea for cases like this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why does it need to be unsafe? You don't need to call useStyleProps at all here, just pass them through and let ActionButton handle it?

<ActionButton {...props}>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah, was trying not to pass through all props in case there was overlap with Menu or MenuTrigger
I suppose in cases like that we'd create a specific object for passing through directly to one of those though, so it'd be fine

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

@devongovett devongovett merged commit 7c7fe44 into main Sep 23, 2021
@devongovett devongovett deleted the add-rest-of-style-props-to-actionmenu branch September 23, 2021 18:11
paulkenney added a commit that referenced this pull request Sep 23, 2021
* main:
  Rename cards packages -> card
  Bump theme packages
  add rest of style props to action menu (#2374)
  Publish
  Add Autocomplete package (#2371)
  CardView followup patch (#2362)
  Creating chromatic baseline for fixing combobox to use useFormProps (#2334)
  Correcting Arabic translations of Loading and Loading more (#2359)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

small review Easy to review PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants