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

package.json: Update @patternfly/patternfly to 5.0.0-prerelease.8 #19111

Merged
merged 21 commits into from
Jul 26, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jul 19, 2023

To do

pixel diff review

Networking

Storage

Looks fine. Some font noise, better dark contrast, and some RTL fixes (in particular, select triangles are now on the correct (left) side)

Expensive

image

Other

Now it is:

image

Note that the margin is now consistent.

Everywhere

Other issues:

  • clicking on the icon in the mobile nav menu does not open the menu. Only clicking on the black space around it or the "System" text works. Regression? Not a regression, handling separately
  • testPrivateKeys failure, due to mis-selecting input; could be related to test: Make Browser.{focus,blur}() synchronous #19078, but most probably not; reproduces effortlessly locally, and robust

Sorry, something went wrong.

@github-actions github-actions bot added the bot label Jul 19, 2023
@github-actions github-actions bot changed the title [no-test] package.json: Update @patternfly/patternfly, @patternfly/react-core, @patternfly/react-icons, @patternfly/react-styles, @patternfly/react-table, @patternfly/react-tokens package.json: Update @patternfly/patternfly, @patternfly/react-core, @patternfly/react-icons, @patternfly/react-styles, @patternfly/react-table, @patternfly/react-tokens Jul 19, 2023
github-actions bot pushed a commit that referenced this pull request Jul 19, 2023
…@patternfly/react-icons, @patternfly/react-styles, @patternfly/react-table, @patternfly/react-tokens

Closes #19111
@github-actions github-actions bot force-pushed the npm-update-patternfly-patternfly-20230719-071632 branch from 06f14d1 to ca68a1a Compare July 19, 2023 07:16
@github-actions

This comment was marked as resolved.

martinpitt pushed a commit that referenced this pull request Jul 19, 2023
…@patternfly/react-icons, @patternfly/react-styles, @patternfly/react-table, @patternfly/react-tokens

Closes #19111
@martinpitt martinpitt force-pushed the npm-update-patternfly-patternfly-20230719-071632 branch from ca68a1a to 4b2c530 Compare July 19, 2023 07:17
@jelly jelly added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jul 19, 2023
@jelly jelly force-pushed the npm-update-patternfly-patternfly-20230719-071632 branch 2 times, most recently from 504ef1b to 6a8e3f0 Compare July 19, 2023 11:47
@cockpit-project cockpit-project deleted a comment from jelly Jul 20, 2023
@martinpitt martinpitt changed the title package.json: Update @patternfly/patternfly, @patternfly/react-core, @patternfly/react-icons, @patternfly/react-styles, @patternfly/react-table, @patternfly/react-tokens package.json: Update @patternfly/patternfly to5.0.0-prerelease.8 , @patternfly/react-core, @patternfly/react-icons, @patternfly/react-styles, @patternfly/react-table, @patternfly/react-tokens Jul 20, 2023
pkg/systemd/terminal.jsx Show resolved Hide resolved
pkg/systemd/terminal.scss Show resolved Hide resolved
@martinpitt martinpitt force-pushed the npm-update-patternfly-patternfly-20230719-071632 branch 3 times, most recently from f8fac6c to 59ce118 Compare July 20, 2023 09:50
@martinpitt martinpitt changed the title package.json: Update @patternfly/patternfly to5.0.0-prerelease.8 , @patternfly/react-core, @patternfly/react-icons, @patternfly/react-styles, @patternfly/react-table, @patternfly/react-tokens package.json: Update @patternfly/patternfly to5.0.0-prerelease.16 , @patternfly/react-core, @patternfly/react-icons, @patternfly/react-styles, @patternfly/react-table, @patternfly/react-tokens Jul 20, 2023
@martinpitt martinpitt force-pushed the npm-update-patternfly-patternfly-20230719-071632 branch from 2f07372 to 093afe0 Compare July 21, 2023 07:16
@martinpitt martinpitt changed the title package.json: Update @patternfly/patternfly to5.0.0-prerelease.16 , @patternfly/react-core, @patternfly/react-icons, @patternfly/react-styles, @patternfly/react-table, @patternfly/react-tokens package.json: Update @patternfly/patternfly to5.0.0-prerelease.8 , @patternfly/react-core, @patternfly/react-icons, @patternfly/react-styles, @patternfly/react-table, @patternfly/react-tokens Jul 21, 2023
@martinpitt
Copy link
Member

I have spent two hours now trying to figure out this mobile nav issue.

First of all, it helps to hack the menu to not close immediately after leaving the mouse, so that one can use the inspector:

diff --git pkg/shell/nav.jsx pkg/shell/nav.jsx
index 203079302..e1be0a5cc 100644
--- pkg/shell/nav.jsx
+++ pkg/shell/nav.jsx
@@ -27,7 +27,7 @@ export const SidebarToggle = () => {
             setActive(false);
         };
 
-        ["blur", "click"].map(ev_type => window.addEventListener(ev_type, handleClickOutside));
+        ["click"].map(ev_type => window.addEventListener(ev_type, handleClickOutside));
 
         return () => {
             ["blur", "click"].map(ev_type => window.removeEventListener(ev_type, handleClickOutside));

But now I'm lost. The inspector tells me that the <span class="nav-item-name">...</span> entries are all 184x24 with zero padding, so these are okay (and also look the same size). The problem is somehow in the parent a.pf-v5-c-nav__link. For that, "Overview" has a content / block height of 32 pixels, while it's 24 for all others. But I have no direct lead to where these extra 8 pixels come from. They seem to come fro the .nav-item-name's ::before or ::after, but these are not directly inspectable.

By trying to unset all parent css items one by one, I found out that it's somehow related to the .pf-v5-c-nav__list, and disabling this:

diff --git pkg/shell/nav.scss pkg/shell/nav.scss
index f471401e0..c3a077155 100644
--- pkg/shell/nav.scss
+++ pkg/shell/nav.scss
@@ -297,6 +297,10 @@ $desktop: $phone + 1px;
     max-block-size: calc(100vh - var(--nav-icon-size));
     max-inline-size: 29rem;
     display: block;
+
+    .pf-v5-c-nav__section {
+      --pf-v5-c-nav__item--MarginTop: 0;
+    }
   }
 
   .nav-hosts-menu {

it "fixes" it in the sense that the items are all aligned. However, then this loses the intentional 8px spacing in between the issues:

Screen Shot 2023-07-21 at 10 41 09

Plus, of course it's a hack. @garrett can I call you for help here? Thanks!

@martinpitt martinpitt changed the title package.json: Update @patternfly/patternfly to5.0.0-prerelease.8 , @patternfly/react-core, @patternfly/react-icons, @patternfly/react-styles, @patternfly/react-table, @patternfly/react-tokens package.json: Update @patternfly/patternfly to 5.0.0-prerelease.8 , @patternfly/react-core, @patternfly/react-icons, @patternfly/react-styles, @patternfly/react-table, @patternfly/react-tokens Jul 21, 2023
@jelly
Copy link
Member

jelly commented Jul 21, 2023

The MTU issue:

image

As we now have to deal with div which can't even be in a label we explicitly set display: inline to make the div sit next to the span but this has as side-effect that the height of the div class="pf-v5-c-form-control mtu-label-input" is now 23pixels instead of the pf-v5-c-radio__label height which is 36px.

I'm not sure what makes it force to 23 pixels, but changing display to anything but inline such as inline-flex makes it show properly.

…@patternfly/react-icons, @patternfly/react-styles, @patternfly/react-table, @patternfly/react-tokens

Remove solved overrides. Also update usage of RedHat font [1].

[1] patternfly/patternfly#5503
@jelly jelly removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jul 25, 2023
@jelly jelly force-pushed the npm-update-patternfly-patternfly-20230719-071632 branch from eea89d8 to 3af9fb0 Compare July 25, 2023 11:22
@jelly jelly removed the .github-changes Set by a reviewer just before landing to acknowledge that a PR changes github workflows label Jul 25, 2023
@martinpitt
Copy link
Member

fcos/networking is very messy, retrying to compare. Looks like the Networking page has trouble loading? But most of it is only at the second or third affected retry run, so probably just CI machine overload.

@martinpitt
Copy link
Member

martinpitt commented Jul 25, 2023

Surprisingly, I can reproduce the ostree failures locally. Curiously it only happens with Chromium, not with Firefox. But when installing the full "chromium" into my toolbox (as I wanted to run it with TEST_SHOW_BROWSER=1), it stops happening.

I refuse to believe that this is really a PatternFly regression. But it's also not chrome-remote-interface. Diffing package-lock.json doesn't point at anything obvious -- a new esbuild plugin and a newer babel.

This is a total ricochet, need to debug this more closely. 🔨

I created a few test PRs to bisect this. Conclusions:

Thus so far it seems that some node module refresh aggravates this, but apparently does not introduce it.

Possible side issue: In this branch, I also get a timeout of this a lot:

  File "/var/home/martin/upstream/cockpit/pf/test/verify/check-networkmanager-basic", line 180, in testIpHelper    
    b.wait_not_present("#network-ip-settings-dialog")    

I watched memory usage of the fcos machine, and it never goes above ~ 40%. This does not look like OOM at first.

I locally bumped the timeout for the frame load to 90s, it doesn't help. So this isn't about "slow".

The evidence thickens that this is somehow a Python bridge regression. #19136 is now this PR plus reverting to the C bridge on F38 and RHEL 9, and it looks quite a bit better.

martinpitt added a commit to martinpitt/cockpit that referenced this pull request Jul 25, 2023
We haven't refreshed node_modules since May, as PR cockpit-project#19111 got stuck for
so long.
@martinpitt martinpitt added needswork blocked Don't land until something else happens first (see task list) and removed needswork labels Jul 25, 2023
martinpitt added a commit to martinpitt/cockpit that referenced this pull request Jul 25, 2023
We haven't refreshed node_modules since May, as PR cockpit-project#19111 got stuck for
so long.
martinpitt added a commit to martinpitt/cockpit that referenced this pull request Jul 25, 2023
We haven't refreshed node_modules since May, as PR cockpit-project#19111 got stuck for
so long.
@martinpitt martinpitt changed the title package.json: Update @patternfly/patternfly to 5.0.0-prerelease.8 , @patternfly/react-core, @patternfly/react-icons, @patternfly/react-styles, @patternfly/react-table, @patternfly/react-tokens package.json: Update @patternfly/patternfly to 5.0.0-prerelease.8 Jul 25, 2023
@martinpitt
Copy link
Member

Executive decision: The above debugging has shown that it's clearly not the PatternFly update which regresses here. This package loading failure has been going on for a while and is related to the Python bridge, and exacerbated by the 3x affected retries.

So, executive decision: Let's land this, and not let another 3 months of work go to waste. I'll debug that bridge bug in #19136 independently, I have a few ideas.

@martinpitt martinpitt removed the blocked Don't land until something else happens first (see task list) label Jul 26, 2023
@martinpitt martinpitt merged commit 5ab2607 into main Jul 26, 2023
@martinpitt martinpitt deleted the npm-update-patternfly-patternfly-20230719-071632 branch July 26, 2023 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants