Skip to content

NavigationLink: set width in order to show caret #20075

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 6, 2020
Merged

NavigationLink: set width in order to show caret #20075

merged 1 commit into from
Feb 6, 2020

Conversation

retrofox
Copy link
Contributor

@retrofox 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.

@retrofox retrofox added [Type] Enhancement A suggestion for improvement. [Block] Navigation Affects the Navigation Block labels Feb 6, 2020
This was referenced Feb 6, 2020
Merged
Merged
jeryj
jeryj approved these changes Feb 6, 2020
View reviewed changes
Copy link
Contributor

@jeryj jeryj left a comment

Choose a reason for hiding this comment

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

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

@retrofox
Copy link
Contributor Author

retrofox commented Feb 6, 2020

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

Keep trying!!!

marekhrabe
marekhrabe approved these changes Feb 6, 2020
View reviewed changes
Copy link
Contributor

@marekhrabe marekhrabe left a comment

Choose a reason for hiding this comment

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

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
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
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
@retrofox retrofox deleted the update/navigation-link-fix-caret-visibility branch February 6, 2020 21:47
@github-actions github-actions bot added this to the Gutenberg 7.5 milestone Feb 6, 2020
@jorgefilipecosta jorgefilipecosta mentioned this pull request Feb 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marekhrabe marekhrabe marekhrabe approved these changes

@jeryj jeryj jeryj approved these changes

@draganescu draganescu Awaiting requested review from draganescu

@ellatrix ellatrix Awaiting requested review from ellatrix

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@Soean Soean Awaiting requested review from Soean

@talldan talldan Awaiting requested review from talldan

Assignees
No one assigned
Labels
[Block] Navigation Affects the Navigation Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
Gutenberg 7.5
Development

Successfully merging this pull request may close these issues.
3 participants
@retrofox @jeryj @marekhrabe