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

NavigationLink: set width in order to show caret #20075

Merged
merged 1 commit into from Feb 6, 2020

Conversation

@retrofox
Copy link
Contributor

retrofox commented Feb 6, 2020

Description

This PR adds a min-width to the label of the Navigation Link when it's empty, and also focused, in order to show the carte.

Fixes the #20073

How has this been tested?

  1. Go to post edit.
  2. Create a Navigation menu.
  3. Add a Navigation Link.
  4. Do not select a link. Just close the popover.
  5. Focus the label in order to start to edit it.

before
6) The caret is hidden

after
7) The caret should be shown.

Screenshots

caret

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
@jeryj
jeryj approved these changes Feb 6, 2020
Copy link
Contributor

jeryj left a comment

Looks good! Works as expected and I couldn't get it to break :)

@retrofox

This comment has been minimized.

Copy link
Contributor Author

retrofox commented Feb 6, 2020

...and I couldn't get it to break :)

Keep trying!!!

Copy link
Contributor

marekhrabe left a comment

Same, works well!

The 20px seems random but considering how the arrow is made (border-width based triangle), I think we are fine using the absolute number here

@jeryj

This comment has been minimized.

Copy link
Contributor

jeryj commented Feb 6, 2020

I tried a few different numbers in there to see the effect, and 20px did end up feeling like the best balance.

@retrofox

This comment has been minimized.

Copy link
Contributor Author

retrofox commented Feb 6, 2020

Thanks for the feedback.
I tried to be more specific using the pseudo :empty but it seems that the element has content despite the label is visually empty.

We could add is-empty CSS class if we consider appropriate.
Thanks for the review.

@retrofox retrofox merged commit f0b4220 into master Feb 6, 2020
3 checks passed
3 checks passed
pull-request-automation
Details
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
Navigation block automation moved this from 👀 PRs to review to ✅ Done Feb 6, 2020
@retrofox retrofox deleted the update/navigation-link-fix-caret-visibility branch Feb 6, 2020
@github-actions github-actions bot added this to the Gutenberg 7.5 milestone Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Navigation block
  
✅ Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.