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

Adjust breadcrumb appearance #2411

Merged
merged 1 commit into from
Dec 21, 2021

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Dec 20, 2021

First step to fix #2410.
The breadcrumb now uses ChevronRight as a separator and has a background as hover feedback.
Screenshot 2021-12-20 at 22-47-26 Inventory - Nextcloud

Peek 21-12-2021 07-55

For a live example, please have a look at the docs:
https://deploy-preview-2411--nextcloud-vue-components.netlify.app/#/Components/Breadcrumbs?id=breadcrumbs-1

I had to rename the component class to vue-crumb since the server CSS kept interfering. If we want to use crumb instead, we will have to explicitly overwrite all interfering server rules.

Still todo see #2410 (comment)

Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Looks great!! 🚀

@skjnldsv skjnldsv added 3. to review Waiting for reviews enhancement New feature or request feature: breadcrumbs Related to the breadcrumbs components labels Dec 21, 2021
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@nimishavijay
Copy link

Looks so much better! Only 2 tiny suggestions regarding colour:

  • in the normal state instead of --color-main-text with 0.7 opacity for the text we could use --color-text-maxcontrast with opacity 1
  • in hovered state we could use --color-main-text for the text with opacity 1 so that there is more difference between the normal and hovered state

What do you think?

@raimund-schluessler
Copy link
Contributor Author

I don't mind. I just tried to resemble the text color currently used in the breadcrumbs, where we have three states: normal, hovered and focused.

  • in the normal state instead of --color-main-text with 0.7 opacity for the text we could use --color-text-maxcontrast with opacity 1

The normal (not hovered, not focuses) uses opacity 0.5 with --color-main-text at the moment. Opacity 0.7 is for hover.

  • in hovered state we could use --color-main-text for the text with opacity 1 so that there is more difference between the normal and hovered state

Hovered is opacity 0.7. Focused uses opacity 1.

So if we use opacity 1 for hovering, we need another way to indicate focus, or make focus and hover look the same. What do you prefer?

@raimund-schluessler
Copy link
Contributor Author

@nimishavijay I hope you don't mind that I merge this now, but it makes it a lot easier to implement the other improvements. I would propose that you do a follow-up PR with your design adjustments. I am fine with whatever @nextcloud/designers agree on.

@raimund-schluessler raimund-schluessler merged commit bf562d8 into master Dec 21, 2021
@raimund-schluessler raimund-schluessler deleted the fix/2410/breadcrumb-appearance branch December 21, 2021 18:46
@jancborchardt
Copy link
Contributor

So if we use opacity 1 for hovering, we need another way to indicate focus, or make focus and hover look the same. What do you prefer?

Yup, would be best if focus and hover look the same. :) And then as @nimishavijay described, not using opacity as modifier for the text color but --color-text-maxcontrast and --color-main-text.

And hey, looks very nice, thank you so much! :)

@marcoambrosini
Copy link
Contributor

marcoambrosini commented Dec 23, 2021

Could we use the button component in the mid run? If we'd do that we'd get the outline focus from it only when navigating with the keyboard and no changes with the mouse.

@raimund-schluessler
Copy link
Contributor Author

raimund-schluessler commented Dec 23, 2021

Could we use the button component in the mid run? If we'd do that we'd get the outline focus from it only when navigating with the keyboard and no changes with the mouse.

I would strongly advise against that.

  • it would add a lot of complexity, because we would need to include the Button in the Breadcrumb component and have an Actions menu inside. I don't even know if that would work.
  • it is syntactically the wrong element. The breadcrumbs are for navigation, so they need to be links/as. Wrapping the button in a link would work, but doesn't make sense, in my opinion.
  • the button component would need adjustment, since the icon is on the wrong side (left for the button, needs to be right here)

I would propose to adjust the Breadcrumb instead to show the desired design. Should be a lot simpler than using Button for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request feature: breadcrumbs Related to the breadcrumbs components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the breadcrumbs component
5 participants