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

Spread modifier styles inside _getPopperStyle and update example #6

Merged
merged 3 commits into from Mar 16, 2017

Conversation

brentertz
Copy link
Contributor

What does this PR do?

Spreads modifier styles inside _getPopperStyle and updates example.

Any background context you want to provide?

We ran into this need when working with a react custom Select component where we want the dropdown list width to match that of the trigger.

The example includes a simpler scenario where we just change the background color.

Screenshot

screen shot 2017-03-09 at 12 32 45 pm

src/Popper.jsx Outdated

return {
position,
top: 0,
left: 0,
transform: `translate3d(${left}px, ${top}px, 0px)`,
willChange: 'transform',
...styles,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, this is similar to how PopperJS handles things inside of its own applyStyle modifier.

https://github.com/FezVrasta/popper.js/blob/master/src/popper/modifiers/applyStyle.js#L53

Copy link
Member

Choose a reason for hiding this comment

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

You don't need any of the above definitions if you are going to use the styles provided by Popper.js, the code here can be simplified.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@FezVrasta so are you saying the transform values and such do not need to be added since styles is used now?

Copy link
Member

Choose a reason for hiding this comment

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

If I'm not missing anything, yup

@souporserious
Copy link
Collaborator

I'd like to keep things natural to React where possible. Your use case sounds like it should be solved by just passing a style attribute to the Popper component. So something like <Popper style={{ width: variableWidth, backgroundColor: 'red' }}/>. The style prop gets merged in with the PopperJS styles here. Please let me know if that works or not.

@FezVrasta
Copy link
Member

The advantage of this change is that someone can use custom modifiers of Popper.js with this component. Otherwise one can only add custom modifiers that leverage on top and left properties.

@souporserious
Copy link
Collaborator

Sorry, I did some updates. Can you rebase master please?

@souporserious souporserious merged commit bf7028d into floating-ui:master Mar 16, 2017
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.

None yet

3 participants