Fixes #30412 - fix shifted react-container #7821

Ron-Lavi · 2020-07-15T08:55:32Z

This is something that I think css @media query would do the work for.
patternfly hides the vertical-nav when the page gets smaller

I think our JS implementation is overkill to what the layout needs to do - to shift to the left when the vertical nav is hidden.
I don't think we need to use the session for it, and I didn't notice anyone using the ON_COLLAPSE/ON_EXPAND actions.

current react pages are shifted:

this is how it looks with this PR:

theforeman-bot · 2020-07-15T08:55:47Z

Issues: #30412

Ron-Lavi · 2020-07-15T08:56:13Z

@sharvit @xprazak2 @MariaAga can you take a look? thanks

sharvit

Thanks @LaViro 👍

We are getting the same features, it fixes a bug and it removes 140 lines of code. Sound like a good deal for me.

sharvit · 2020-07-15T09:15:55Z

app/assets/stylesheets/base.scss

+  patternfly-sass hides the vertical-nav on smaller screens, see: https://github.com/patternfly/patternfly-sass/blob/5e7cce3445d5b1af1e16500695b9f33edc6a4f6c/assets/javascripts/patternfly-functions.js#L260
+  the react-container needs to be shifted when it happens.
+*/
+@media only screen and (max-width: 768px) {


Can we put it in webpack and import the pf3 variable for the max-width?

thanks, added those in my last commit

sharvit · 2020-07-15T09:21:31Z

Look like it we still have it in the session storage, does patternfly manage that internally?

MariaAga · 2020-07-15T10:58:59Z

@LaViro Do you think it will work with the Nav in PF4? #7809

app/assets/stylesheets/base.scss

Ron-Lavi · 2020-07-15T11:59:42Z

Look like it we still have it in the session storage, does patternfly manage that internally?

@sharvit after doing a hard reset to the browser I don't see it in the session anymore:

Do you think it will work with the Nav in PF4? #7809

@MariaAga If they use the same scss classnames, it even make the implementation easier because
you won't need to take care of it in the JS part, but still check the full-react pages to see if it works.

sharvit · 2020-07-15T12:13:07Z

Look like it we still have it in the session storage, does patternfly manage that internally?

@sharvit after doing a hard reset to the browser I don't see it in the session anymore:

Do you think it will work with the Nav in PF4? #7809

@MariaAga If they use the same scss classnames, it even make the implementation easier because
you won't need to take care of it in the JS part, but still check the full-react pages to see if it works.

From the other side, I don't think pf4 has the implementation for collapsing the menu so we might want to keep the logic?

Ron-Lavi · 2020-07-15T12:16:09Z

From the other side, I don't think pf4 has the implementation for collapsing the menu so we might want to keep the logic?

@MariaAga is it correct?
in that case I would just add a css rule to hide the vertical nav too, when they both use the same variable for md grid breakpoint.

MariaAga · 2020-07-15T13:00:35Z

There is an option to open/close the menu in PF4,I use oncollapse to move the page content (forexample host table) to the right of the menu and not under it when it's open

sharvit · 2020-07-15T13:05:50Z

@LaViro the reason for the sessionStorage is to keep the state of the menu when you are refreshing the page or moving to a different page (if it's not using client-routing).
Looks like it is still working and I still see the sessionStorage after removing it and refreshing the page. Maybe we have something on the server?

Ron-Lavi · 2020-07-15T13:11:59Z

There is an option to open/close the menu in PF4,I use oncollapse to move the page content (forexample host table) to the right of the menu and not under it when it's open

I still think in this case we should use only css, based of the width of the screen, look how much code does it reduce.
you can do it simply with JS too, but preferably css.

@LaViro the reason for the sessionStorage is to keep the state of the menu when you are refreshing the page or moving to a different page (if it's not using client-routing).
Looks like it is still working and I still see the sessionStorage after removing it and refreshing the page. Maybe we have something on the server?

interesting, I didn't see any usecase of "navCollapsed" or "pinnedPath" which are the values that was set in the session,
I think that the menu gets the "active" menu data from the server, based on the route

do you have any other concerns? I didn't find any regression

MariaAga · 2020-07-15T13:18:54Z

Generally I'm not sure it's a good idea to add PRs right now to the old PF3 layout with PF3 rules when we have a PR to replace it with PF4

Ron-Lavi · 2020-07-15T14:47:33Z

Well currently there is a bug in all of this pages,
Looks like the react implementation doesn't work atm
In your new PR you can add a simpler solution than was before, but lets fix the issue first and remove unused code.

sharvit

Thanks @LaViro 👍

Looks like the pf3 component manages the session-storage for us, see:
https://github.com/patternfly/patternfly-react/blob/patternfly-3/packages/patternfly-react/src/components/VerticalNav/VerticalNav.js#L120

sharvit · 2020-07-15T15:21:29Z

webpack/assets/javascripts/react_app/common/grid-breakpoints.scss

+$pf-global--breakpoint--md: 768px !default;
+$pf-global--breakpoint--lg: 992px !default;
+$pf-global--breakpoint--xl: 1200px !default;
+$pf-global--breakpoint--2xl: 1450px !default;


We should be able to import them from vendor, see:
https://github.com/theforeman/foreman-js/blob/master/packages/vendor-core/scss/variables.scss
https://github.com/theforeman/foreman/blob/develop/webpack/assets/javascripts/react_app/components/AutoComplete/auto-complete.scss#L1

nice thanks! I will change it.
while we are at it, is it possible to make those variables available all over the app without the import?
can we do the same for colors variables?

Ron-Lavi · 2020-07-15T16:40:18Z

Looks like the pf3 component manages the session-storage for us, see:
https://github.com/patternfly/patternfly-react/blob/patternfly-3/packages/patternfly-react/src/components/VerticalNav/VerticalNav.js#L120

interesting, is that a good practice?

sharvit

I am getting this layout issue in the hw-models page:

sharvit · 2020-07-16T11:02:16Z

interesting, is that a good practice?

I think so, that way we can keep it in the same state when moving between pages

MariaAga · 2020-07-16T12:49:05Z

To solve this I think it be easier to just change:

foreman/webpack/assets/javascripts/react_app/components/Layout/LayoutSessionStorage.js

Line 4 in 0c9ddcc

!!getValue(`["navCollapsed","pinnedPath"]`);

to

return !!getValue(`["navCollapsed","pinnedPath"]`).navCollapsed;

And then we don't have to add more PF3 to our code...
Also with this I don't see Avi's issue

Ron-Lavi · 2020-07-16T13:24:09Z

I am getting this layout issue in the hw-models page

Thanks! I forgot we can do that :) :)
so not much of code removal this time..

thanks @MariaAga, added your session suggestion though the container was shifted 200px to the right on small screens when the vertical nav is hidden.
so I kept the css changes and the move of them to webpack,

ready for another review 👍

MariaAga · 2020-07-16T14:14:00Z

webpack/assets/javascripts/react_app/components/Layout/layout.scss

+  @media only screen and (max-width: $pf-global--breakpoint--md) {
+    .react-container {
+      margin-left: 0;
+    }
+  }


This is getting overridden by the previous rule:
&.collapsed-nav { margin-left: 75px; }
as #react-app-root is an ID so the rule is more specific

done, fixed the situation where screen was small and nav collapsed 👍

Ron-Lavi · 2020-07-16T17:40:05Z

How I love css 🤣

added another fix, ready for another review
this is how the page reacts now ( sorry for the low quality )

jeremylenz

Thanks @LaViro, tested on some foreman pages and also Katello Angular and React pages. works great!

Just one nit below

jeremylenz · 2020-07-16T20:24:45Z

webpack/assets/javascripts/react_app/components/Layout/LayoutSessionStorage.js

@@ -1,4 +1,4 @@
 import { getValue } from '../../common/SessionStorage';

 export const getIsNavbarCollapsed = () =>
-  !!getValue(`["navCollapsed","pinnedPath"]`);
+  !!getValue(`["navCollapsed","pinnedPath"]`)?.navCollapsed;


Optional chaining party! 🎉

Prefer adding parenthesis to avoid ambiguity (does the !! apply to everything?)

Suggested change

!!getValue(`["navCollapsed","pinnedPath"]`)?.navCollapsed;

!!(getValue(`["navCollapsed","pinnedPath"]`)?.navCollapsed);

no need to add !! operator
the navCollapsed is already a boolean
btw, this line caused the regression with the vertical navigation
Thanks for the @media extra feature :)

no need to add !! operator
the navCollapsed is already a boolean

before the user clicks to collapse the menu, the values are initially set to null by us
I guess that's why !! was used,to make sure it's a boolean.

works for me without the !!
null is still a false value in conditions

ok lets do this.. since it's only being used to set a className it's indeed not important if it's null or false

oh no.. removing it just failed 4 snapshots in travis
showing undefined instead of false.. I am reverting it so the snapshot will stay the same

@jeremylenz I tried to add those brackets you ask for, but the prettier linter is annoyed by that and ask to remove it :(

If we can't ignore the lint rule I'd go for updating the snapshots. But either way this isn't a big enough deal to be a blocker, if it has to remain without parenthesis then I guess I'll live with it 🤷‍♂️

I prefer a snapshot with the prop as false instead of undefined
because it is not clear whether the prop wasn't passed at all, or was it pass as undefined?
(in our case it was passed as undefined)

MariaAga

React and not React margin works correctly now 🎉

Ron-Lavi · 2020-07-20T08:49:52Z

rebased as Refactor the Layout component got merged,
everything seem to work fine, any other suggestions before merging?

coveralls · 2020-07-20T09:10:28Z

Coverage remained the same at 74.617% when pulling ec7832b on laviro:fix/react_container_margin into e4c39a7 on theforeman:develop.

Requested changes addressed.

ezr-ondrej

Works for me, test failure unrelated.
Thanks @LaViro and @sharvit, @jeremylenz, @MariaAga and @amirfefer for reviews! 👍

theforeman-bot added Needs testing Not yet reviewed UI labels Jul 15, 2020

sharvit suggested changes Jul 15, 2020

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Jul 15, 2020

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jul 15, 2020

houndci-bot reviewed Jul 15, 2020

View reviewed changes

sharvit approved these changes Jul 15, 2020

View reviewed changes

theforeman-bot removed the Needs re-review label Jul 15, 2020

sharvit suggested changes Jul 15, 2020

View reviewed changes

theforeman-bot added the Waiting on contributor label Jul 15, 2020

Ron-Lavi force-pushed the fix/react_container_margin branch from 0083cab to 12c3bf7 Compare July 15, 2020 17:01

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jul 15, 2020

sharvit previously requested changes Jul 16, 2020

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Jul 16, 2020

MariaAga mentioned this pull request Jul 16, 2020

Fixes #30044 - Host Details Page Redesign #7726

Merged

Ron-Lavi force-pushed the fix/react_container_margin branch from 12c3bf7 to 769b0d2 Compare July 16, 2020 13:22

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jul 16, 2020

Ron-Lavi changed the title ~~Fixes #30412 - use css rule instead of JS implementation for vertical nav collapse~~ Fixes #30412 - fix shifted react-container Jul 16, 2020

MariaAga reviewed Jul 16, 2020

View reviewed changes

Ron-Lavi force-pushed the fix/react_container_margin branch from 769b0d2 to 742d98a Compare July 16, 2020 17:36

jeremylenz reviewed Jul 16, 2020

View reviewed changes

MariaAga previously approved these changes Jul 19, 2020

View reviewed changes

theforeman-bot removed the Needs re-review label Jul 19, 2020

Ron-Lavi dismissed MariaAga’s stale review via e497c06 July 20, 2020 08:47

Ron-Lavi force-pushed the fix/react_container_margin branch from 742d98a to e497c06 Compare July 20, 2020 08:47

Fixes #30412 - fix shifted react-container

ec7832b

Ron-Lavi force-pushed the fix/react_container_margin branch from e497c06 to ec7832b Compare July 20, 2020 08:58

ezr-ondrej approved these changes Jul 21, 2020

View reviewed changes

ezr-ondrej merged commit d2b79df into theforeman:develop Jul 21, 2020

	!!getValue(`["navCollapsed","pinnedPath"]`)?.navCollapsed;
	!!(getValue(`["navCollapsed","pinnedPath"]`)?.navCollapsed);

Fixes #30412 - fix shifted react-container #7821

Fixes #30412 - fix shifted react-container #7821

Conversation

Ron-Lavi commented Jul 15, 2020

theforeman-bot commented Jul 15, 2020

Ron-Lavi commented Jul 15, 2020

sharvit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharvit commented Jul 15, 2020

MariaAga commented Jul 15, 2020

Ron-Lavi commented Jul 15, 2020

sharvit commented Jul 15, 2020

Ron-Lavi commented Jul 15, 2020

MariaAga commented Jul 15, 2020 • edited Loading

sharvit commented Jul 15, 2020

Ron-Lavi commented Jul 15, 2020

MariaAga commented Jul 15, 2020

Ron-Lavi commented Jul 15, 2020

sharvit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ron-Lavi commented Jul 15, 2020

sharvit left a comment

Choose a reason for hiding this comment

sharvit commented Jul 16, 2020

MariaAga commented Jul 16, 2020 • edited Loading

Ron-Lavi commented Jul 16, 2020

MariaAga Jul 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ron-Lavi commented Jul 16, 2020

jeremylenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer Jul 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MariaAga left a comment

Choose a reason for hiding this comment

Ron-Lavi commented Jul 20, 2020

coveralls commented Jul 20, 2020

ezr-ondrej left a comment

Choose a reason for hiding this comment

MariaAga commented Jul 15, 2020 •

edited

Loading

MariaAga commented Jul 16, 2020 •

edited

Loading

MariaAga Jul 16, 2020 •

edited

Loading

amirfefer Jul 17, 2020 •

edited

Loading

amirfefer Jul 19, 2020 •

edited

Loading