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

Fix indent of the groupped toolbar #1489

Merged
merged 1 commit into from
Jan 20, 2022
Merged

Conversation

brumik
Copy link
Contributor

@brumik brumik commented Jan 10, 2022

Issue: https://issues.redhat.com/browse/AAH-822

Concern about the change

If we edit these pages (merge this PR), then the toolbar indent is not matching to the "Approval" and "User access" page's toolbar (Where they have this indent).

Before

image

Result

Screenshot from 2022-01-10 10-43-08
Screenshot from 2022-01-10 10-43-00

@brumik brumik requested a review from himdel January 10, 2022 09:46
@brumik brumik force-pushed the brumik-fixing-filter-css-AAH-822 branch 2 times, most recently from b87f5c0 to b85c804 Compare January 10, 2022 16:37
@brumik
Copy link
Contributor Author

brumik commented Jan 10, 2022

If we edit these pages (merge this PR), then the toolbar indent is not matching to the "Approval" and "User access" page's toolbar (Where they have this indent).

Taufique Rahman approved and we may raise this issue to the PF team.

Good to merge

@brumik brumik force-pushed the brumik-fixing-filter-css-AAH-822 branch 3 times, most recently from 9d014ff to 8b36601 Compare January 11, 2022 13:56
@himdel
Copy link
Collaborator

himdel commented Jan 11, 2022

If we edit these pages (merge this PR), then the toolbar indent is not matching to the "Approval" and "User access" page's toolbar (Where they have this indent).

Those should also be fixed, I believe the goal was to fix all toolbars to align with the header.
(The issue is from July, we didn't have Execution Environments nor Task Management back then)

But up to you if you want to keep this one short and deal with the rest separately :)

@brumik
Copy link
Contributor Author

brumik commented Jan 12, 2022

@himdel I looked into that issue and found the global solution for it. It was a random pf class overwrite in a scss affecting every page. Ideally we should not overwrite pf classes in scss. (In fact I am a big fan of styled-components since it encapsulates the styling and makes these kinds of problems disapear :)

Anyway: I feel like your concerns on this PR are answered. Another round of review?

@himdel
Copy link
Collaborator

himdel commented Jan 13, 2022

OK, looks like we need to keep the class override for now, this now breaks the alignment on those screens...
20220113004251
20220113004228

@brumik
Copy link
Contributor Author

brumik commented Jan 13, 2022

It would have been helpful to get full screenshots so I know where is this occurring. We can overwrote them one by one rather than overriding the pf class and then override it back on places where we don't need it.

Edit: the toolbar actually should more or less look like we have, we have more overrides. Let me go trough them.

@brumik
Copy link
Contributor Author

brumik commented Jan 13, 2022

I modified most of the toolbars to:

  1. Keep the current looks and the fixed paddings
  2. Use minimal scss and custom styling and follow the PF library as close as possible.

This modification included

  1. making the custom css to PF
  2. put the pagination into the toolbar
  3. put the chips under the toolbar while keeping in the toolbar (will help in the future when starting to use PF automatic chips)

All the modified toolbars are visible in the screenshots under.

Followup task could be:

  1. make the chips under the toolbar use the default patternfly builtin functions instead of hacking it under the toolbar by ourselves. (If I get the time I would like to work on this.)

Screenshot from 2022-01-13 11-32-25
Screenshot from 2022-01-13 11-32-15
Screenshot from 2022-01-13 11-31-59
Screenshot from 2022-01-13 11-31-45
Screenshot from 2022-01-13 11-31-29
Screenshot from 2022-01-13 11-31-15

@brumik brumik force-pushed the brumik-fixing-filter-css-AAH-822 branch from 7f1f6eb to d50a022 Compare January 13, 2022 10:50
@himdel
Copy link
Collaborator

himdel commented Jan 13, 2022

So, the original change was really close, this now seems overly ambitious for that particular bug, especially since we may want to backport the fix.

Can you split this into 2 separate PRs please? Or just revert back to the original fix for Collections, without affecting the rest of the application.


More specifically, this now seems to be introducing quite some extra whitespace in both layouts...

master here
empty 20220113212708 20220113212456
filtering 20220113212651 20220113212514

Apart from fixing AppliedFilters alignment in Remote Registries :)...

master here
empty 20220113214028 20220113213838
filtering 20220113214017 20220113213905

@brumik
Copy link
Contributor Author

brumik commented Jan 14, 2022

@himdel Ill do a quick fix then and leave this on the side.

The filter spacing can be not so easily fixed by using the built in filters in the PF Toolbar component.
When I do these changes I can redo that part too, but that's a bit bigger refactor (I am happy to undertake but may take up more time so I need somebody approval that I can work on it)

@brumik brumik force-pushed the brumik-fixing-filter-css-AAH-822 branch from d50a022 to 5be2893 Compare January 14, 2022 08:45
@brumik
Copy link
Contributor Author

brumik commented Jan 14, 2022

For the current state look at the PR description.

@brumik brumik force-pushed the brumik-fixing-filter-css-AAH-822 branch from 5be2893 to 9749580 Compare January 14, 2022 08:48
@brumik brumik requested a review from himdel January 14, 2022 08:49
@brumik brumik merged commit d950246 into master Jan 20, 2022
@brumik brumik deleted the brumik-fixing-filter-css-AAH-822 branch January 20, 2022 09:02
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.

None yet

2 participants