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

Update PF to version 5.1 and fix some alignment UI issues #6836

Merged
merged 12 commits into from
Nov 22, 2023

Conversation

ferhoyos
Copy link
Contributor

@ferhoyos ferhoyos commented Nov 10, 2023

Describe the change

This PR upgrades Patternfly to version 5.1 and fix some UI issues:

  • Info icons not aligned properly (Info icon in yaml editor's overview panel is not aligned properly #6773)
  • Formatting and fix some CSS style disalignments (margin and paddings)
  • The Close button does not work on Debug Information.
  • Type column sorting does not seem to work for Envoy -> Clusters subtab
  • Traffic tab sorting seems to only sort one way
  • Same health icon size in all Kiali tables

image

  • Change some colors so that it looks better on Dark mode:

    • Graph Replay controls
    • Metrics chart background stroke
    • 'Edit in Kiali' link in Istio config page (only for kiosk)
  • More legend overlay disalignment

image

  • Since ServiceValidationSummary component is very similar to ValidationSummary component and Class inheritance is not allowed with React Hooks, I have added new type property to ValidationSummary (istio or service) and delete ServiceValidationSummary component.

  • NamespaceInfo.ts moved to types folder

  • HealthStyle moved to styles folder

  • Remove some console.log in Renderers component

Refactorization Bonus (in files modified):

  • Small components migrated to hooks (plain components, straightforward conversions).
  • Alphabetized props lists
  • Add method types (params and return)
  • Replace strings with correct concatenation (Replace strings with correct concatenation #6736)
  • Replace || by ?? to check nullish values (|| operator can lead to errors since it checks falsy values, not nullish)
  • Convert function to arrow function when it is possible
  • Convert var to let or constant when it is possible

** Issue reference **

Closes #6768

@ferhoyos
Copy link
Contributor Author

@jshaughn One of the issues you found in one of previous PR reviews is confusing me...

  • Traffic tab sorting seems to only sort one way

I have fixed it but now both inbound and outbound tables are connected in the sorting handling. I mean that if you sort inbound traffic table by name, the outbound traffic table is also sorted. Besides, if you see the in the url there are only one sort and direction URL params. Is this the expected behaviour or both tables should have separate sorting handlers?

image

image

@jshaughn
Copy link
Collaborator

Is this the expected behaviour or both tables should have separate sorting handlers?

Right, I think I remember this as always being an issue for this page. I think the decision was that it is OK if the sort affects both tables as it was not worth the added complexity to make it two different sorts. So, I think this is fine as is!

@ferhoyos ferhoyos changed the title Update PF to version 5.1 and fix little UI issues Update PF to version 5.1 and fix some alignment UI issues Nov 20, 2023
@ferhoyos ferhoyos marked this pull request as ready for review November 20, 2023 17:19
@ferhoyos ferhoyos self-assigned this Nov 20, 2023
Copy link
Collaborator

@jshaughn jshaughn left a comment

Choose a reason for hiding this comment

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

Almost everything looks good to me! Tested the oddities that I knew about and everything seems fixed. Looked at some other fixes and everything was good/better. One new issue, in the PF graph the legend now has icon+text, which makes the legend too verbose:

image

@ferhoyos
Copy link
Contributor Author

I've been investigating the issue and the text should not appear.

The text element within the legend is:

<span class="pf-v5-u-screen-reader">Zoom Out</span>

According to https://www.patternfly.org/utility-classes/accessibility/#usage, CSS class pf-v5-u-screen-reader should hide the text but leaves accessible to assistive technologies. That means we are not loading all Patternfly CSS. I will check how to load PF accessibility CSS.

@aljesusg
Copy link
Collaborator

Almost everything looks good to me! Tested the oddities that I knew about and everything seems fixed. Looked at some other fixes and everything was good/better. One new issue, in the PF graph the legend now has icon+text, which makes the legend too verbose:

image

Could we add some padding betweenthe icon and text ?

@ferhoyos
Copy link
Contributor Author

Yes, we could add padding but I think the best approach here (and the intention of react-topology library) is to show only the icons (then tooltip gives more information about the toolbar option). This is the approach we follow in Cystoscape graph and patternfly examples (https://www.patternfly.org/topology/control-bar)

@aljesusg
Copy link
Collaborator

Yes, we could add padding but I think the best approach here (and the intention of react-topology library) is to show only the icons (then tooltip gives more information about the toolbar option). This is the approach we follow in Cystoscape graph and patternfly examples (https://www.patternfly.org/topology/control-bar)

We have so buttons with the same icon...we'll need help here from UX for this if we want to have only icons

@ferhoyos
Copy link
Contributor Author

Sure, we can ask UX for guiding in the topology legend, only thing is that we would have to follow same approach for both graphs (cytoscape and PF).

@ferhoyos
Copy link
Contributor Author

I have loaded PF utilities CSS and now the text is hidden in PF graph legend:

image

@jshaughn
Copy link
Collaborator

Sure, we can ask UX for guiding in the topology legend, only thing is that we would have to follow same approach for both graphs (cytoscape and PF).

We can ask for guidance but I will veto anything that has on-screen text, it needs to be handled with icons and tooltips. The alternative is too verbose.

Copy link
Collaborator

@jshaughn jshaughn left a comment

Choose a reason for hiding this comment

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

Now that the ControlBar (on-graph icons) is back to having no text, just icons, I approve! Nice work.

Copy link
Contributor

@hhovsepy hhovsepy left a comment

Choose a reason for hiding this comment

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

Regression tested the pages, compared the alignment with older one, now it looks better and all small issues with icons alignments are resolved. Thank you @ferhoyos for the massive work.

@ferhoyos ferhoyos merged commit 790dcef into kiali:master Nov 22, 2023
7 checks passed
@ferhoyos ferhoyos deleted the 6768-patternfly-5.1 branch November 22, 2023 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Update Patternfly library to version 5.1
4 participants