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

Move breadcrumbs out of location picker heading #5020

Merged

Conversation

LukasHirt
Copy link
Collaborator

@LukasHirt LukasHirt commented Apr 26, 2021

Description

We've moved the breadcrumbs element out of the location picker heading and moved it under it.
The heading is now also reflecting the page title.
We've also decreased the size of both breadcrumbs and action buttons so that they fit better together.

Screenshots (if appropriate):

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

@LukasHirt LukasHirt added the Category:Enhancement Add new functionality label Apr 26, 2021
@LukasHirt LukasHirt self-assigned this Apr 26, 2021
@update-docs
Copy link

update-docs bot commented Apr 26, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@LukasHirt
Copy link
Collaborator Author

I'm trying to think about the table... it has different padding than anything above it. Updating ODS with prop for padding of the table would again just increase the effort & time needed but I do not want to decrease the padding from elements above as that looks like 💩

@LukasHirt LukasHirt changed the base branch from master to a11y-swarming April 26, 2021 11:35
@kulmann
Copy link
Member

kulmann commented Apr 26, 2021

There's more - the horizontal line above has a slight margin left and right, while the borders of the table go from left to right without margin. Could you check if applying that same margin to the left and right of the table makes it look better? If not, then I'd be all in for making the padding left and right slightly bigger in the ODS component.

@kulmann
Copy link
Member

kulmann commented Apr 26, 2021

In the All files view we now have the variation="lead"for the breadcrumbs. Could you check if you can use the same here? I think that the variation="lead" has a font size that it too big for the use case. Would prefer something in the middle. Smaller than lead and bigger than regular. Maybe introduce a third option?

@LukasHirt LukasHirt force-pushed the feat/move-breadcrumbs-out-of-location-picker-heading branch from 8458526 to eed3960 Compare April 26, 2021 12:13
@ownclouders
Copy link
Contributor

💥 Acceptance tests Move failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/web/15125/

20210426-122135-777.png
20210426-122311-513.png
20210426-122417-383.png
20210426-122455-077.png
20210426-122538-346.png
20210426-122659-758.png
20210426-122713-867.png

@LukasHirt
Copy link
Collaborator Author

@LukasHirt LukasHirt force-pushed the feat/move-breadcrumbs-out-of-location-picker-heading branch from 4c2cb18 to d24b543 Compare April 26, 2021 14:25
@LukasHirt
Copy link
Collaborator Author

@kulmann I would say it's ready for review. We need an ODS update for the padding improvement, but we do not need to touch anything after that so I wouldn't really block this PR because of that.

@ownclouders
Copy link
Contributor

💥 Acceptance tests Move failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/web/15128/

20210426-143025-609.png
20210426-143155-535.png
20210426-143258-350.png
20210426-143335-664.png
20210426-143423-423.png
20210426-143547-435.png
20210426-143600-307.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests Move failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/web/15129/

20210426-143738-731.png
20210426-143919-302.png
20210426-144027-843.png
20210426-144106-365.png
20210426-144154-160.png
20210426-144318-681.png
20210426-144332-274.png

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

LGTM

@kulmann kulmann merged commit dc3e760 into a11y-swarming Apr 26, 2021
@delete-merged-branch delete-merged-branch bot deleted the feat/move-breadcrumbs-out-of-location-picker-heading branch April 26, 2021 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Enhancement Add new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants