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

ODC-6064: Update PatternFly to fix unexpected spacing below topology toolbar #9453

Merged
merged 1 commit into from Jul 15, 2021

Conversation

jerolimov
Copy link
Member

@jerolimov jerolimov commented Jul 8, 2021

Fixes:
https://issues.redhat.com/browse/ODC-6064

Analysis / Root cause:
With one of the latest @patternfly/react-core the Toolbar component renders an unexpceted spacing. See before screenshots below.

Solution Description:
Update @patternfly/patternfly and @patternfly/react-core to the latest version which includes the bugfix

See also

Screen shots / Gifs for design review:

Broken topology toolbar before:

image

Topology toolbar now (again without spacing):

image

Search for other UI changes.

The only differences I noticed yet is that the perspective dropdown shows now a gray bottom-border line which changes to blue if the user hovers it.

Before:

Peek 2021-07-09 00-25

After (without the change in _nav_header.scss, with the change there is no visual change to the old version):

Peek 2021-07-09 00-38

With the latest @patternfly/patternfly update this issue is fixed again. I don't find any other layout issues yet.

Unit test coverage report:
Untouched

Test setup:
Just open the developer perspective > topology.

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

/kind bug

@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Jul 8, 2021
@openshift-ci openshift-ci bot requested review from jcaianirh and rhamilto July 8, 2021 22:31
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2021
@jerolimov
Copy link
Member Author

jerolimov commented Jul 8, 2021

I think we should wait for UX feedback if the perspective switch change is a bug or a feature...

It is at least the default behavior of a PatternFly dropdown https://www.patternfly.org/v4/components/dropdown

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 8, 2021
@rhamilto
Copy link
Member

rhamilto commented Jul 9, 2021

Search for other UI changes. The only differences I noticed yet is that the perspective dropdown shows now a gray bottom-border line which changes to blue if the user hovers it.

I noticed this as well and reported it at patternfly/patternfly#4155 (comment) (which I now realize wasn't the offending PR, but the issue links to the offending PR). I worked around the issue with https://github.com/openshift/console/pull/9419/files#diff-8f2141c9b030221e65e6a3c024604a13181de9046fdc1f54c60e23b6a10c045eR14-R18

@rhamilto
Copy link
Member

rhamilto commented Jul 9, 2021

Fix for the breaking PatternFly change coming. See patternfly/patternfly#4155 (comment)

@openshift-ci openshift-ci bot added the component/core Related to console core functionality label Jul 12, 2021
@jerolimov
Copy link
Member Author

jerolimov commented Jul 12, 2021

@rhamilto thanks for these links.

For this PR I removed the border also from for the :after element (see _nav-header.scss change). So that there is no visual difference anymore to the old version. From my side this PR is fine with this change, but let me know what you think. :)

I remove the hold as I don't know any visual blocker at the moment. I also updated PF again and rebased this PR.
/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2021
@rhamilto
Copy link
Member

@rhamilto thanks for these links.

For this PR I removed the border also from for the :after element (see _nav-header.scss change). So that there is no visual difference anymore to the old version. From my side this PR is fine with this change, but let me know what you think. :)

I remove the hold as I don't know any visual blocker at the moment. I also updated PF again and rebased this PR.
/unhold

PatternFly is to revert the changes requiring you to add :after to the existing rule. I think it would be best to wait until those changes land so there is no need to add to the existing override as once the reversion is released, the :after addition will be unnecessary.

@jerolimov
Copy link
Member Author

OK. Waiting for

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2021
@jerolimov
Copy link
Member Author

jerolimov commented Jul 14, 2021

@rhamilto patternfly/patternfly#4213 is merged and released. I updated this PR again to the latest PF releases.

The topology toolbar is still fine with this PR, the perspective switch looks good again (without any CSS change in this PR anymore). And I could not find any other UI glitch at the moment.

From my side we could update PF now. Look good to you as well?

Are you fine with adding such a update-patternfly.sh script so that everyone knows how to update it (use prerelease instead latest, -W, --exact) 😄 ?

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 14, 2021
@rhamilto
Copy link
Member

From my side we could update PF now. Look good to you as well?

Yep!

Are you fine with adding such a update-patternfly.sh script so that everyone knows how to update it (use prerelease instead latest, -W, --exact) 😄 ?

I'll defer to the @spadgett and @christianvogt on that one.

@christianvogt
Copy link
Contributor

If you want to deliver a script because it makes the task easier. go for it.

Copy link
Member

@rhamilto rhamilto left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 14, 2021
@rhamilto rhamilto changed the title Update PatternFly to fix unexpected spacing below topology toolbar ODC-6064: Update PatternFly to fix unexpected spacing below topology toolbar Jul 14, 2021
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 14, 2021
@jerolimov jerolimov requested a review from rhamilto July 14, 2021 22:03
Copy link
Member

@rhamilto rhamilto left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 14, 2021
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2021
@rhamilto
Copy link
Member

Sorry, @jerolimov. It looks like my PR landed before yours did, and now you need to rebase.

@openshift-ci openshift-ci bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 15, 2021
@jerolimov
Copy link
Member Author

Sorry, @jerolimov. It looks like my PR landed before yours did, and now you need to rebase.

No problem. Rebased, re-run the script and done. ;)

@jerolimov jerolimov requested a review from rhamilto July 15, 2021 16:01
Copy link
Member

@rhamilto rhamilto left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 15, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerolimov, rhamilto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit a93299b into openshift:master Jul 15, 2021
@spadgett spadgett added this to the v4.9 milestone Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants