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: 🎸 auto position combined link actions dropdown #453

Merged
merged 1 commit into from
Oct 28, 2020

Conversation

DanweDE
Copy link
Contributor

@DanweDE DanweDE commented Oct 22, 2020

The new combined reference link action's dropdown will be positioned automatically, if necessary, above the trigger button (in most cases, if there's enough space, it will still be shown below). This should prevent the menu from being cut off when e.g. positioned at the end of a page.

@m10l
Copy link
Contributor

m10l commented Oct 22, 2020

@DanweDE I'd hold off on this - there's a PR open to refactor the dropdown component to use a more stable positioning package (popper.js) - contentful/forma-36#597

@DanweDE
Copy link
Contributor Author

DanweDE commented Oct 22, 2020

@m10l this PR shouldn't conflict with that work, but without it we wouldn't benefit from the positioning in contentful/forma-36#597 as auto-positioning is currently disabled on this button. Or am I missing something?

Seems like that PR is removing the isAutoalignmentEnabled altogether. If that'll really be the final version, we won't need this PR anymore (merging it now wouldn't harm though) but should clean-up the use of the prop.

The new combined reference link action's dropdown will be positioned
automatically, if necessary, above the trigger button. This should
prevent the menu from being cut off when e.g. positioned at the end of a
page.
@DanweDE DanweDE force-pushed the fix-combined-link-actions-auto-position branch from fee2327 to 6df21bf Compare October 22, 2020 16:48
@DanweDE DanweDE requested a review from denkristoffer October 26, 2020 12:55
@DanweDE
Copy link
Contributor Author

DanweDE commented Oct 26, 2020

Since the isAutoalignmentEnabled prop is celebrating its comeback in contentful/forma-36#597 I think we can just merge this PR @denkristoffer @m10l

I think I managed to replicate disabling positioning with the isAutoAlignmentEnabled prop. I have un-deprecated it, so we can get this in without breaking changes. I think it's ready.

@denkristoffer
Copy link
Contributor

@DanweDE Looks right, but the values passed to the dropdown are also the default values, so are you sure this commit changes anything?

@DanweDE
Copy link
Contributor Author

DanweDE commented Oct 27, 2020

but the values passed to the dropdown are also the default values

@denkristoffer Pretty sure this is necessary, see this line where the default is false, not the actual dropdown default which would be true.

@suevalov
Copy link
Contributor

@denkristoffer @DanweDE should it be closed by #466?

@denkristoffer
Copy link
Contributor

@suevalov No it looks like the "Create entry" dropdown disables automatic positioning by default, so this is a good change.

@DanweDE DanweDE merged commit d5f4f8b into master Oct 28, 2020
@DanweDE DanweDE deleted the fix-combined-link-actions-auto-position branch October 28, 2020 07:37
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