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

Convert trees to react #6413

Merged
merged 21 commits into from
May 20, 2020
Merged

Convert trees to react #6413

merged 21 commits into from
May 20, 2020

Conversation

brumik
Copy link

@brumik brumik commented Nov 14, 2019

Tree updates

Converts the trees to the new react tree, adding all the required functionality:

  • post check
  • onCheck handlers
  • onClick handlers
  • lazy load
  • check all
  • select
  • select silent
  • scroll to node
  • expand (by reducer)
  • expand all (by reducer)

Affected trees (at least) by screens

  • Settings -> Region -> CU Collection tab
  • Compute -> Ingrastructure -> VMs -> Select one vm and click on genealogy
  • Settings -> Access Controll -> Roles/Edit Roles // Seems like not doing anything
  • Compute -> Infra -> VMs -> Select VM -> Policy button -> edit
  • Compute -> Infra -> VMs -> Compare VM's
  • Automate -> Expoler -> Select a class -> Copy -> Uncheck Copy to same path -> Namespace selection uses thath tree
  • Services -> Catalogs -> Catalog Items -> add a new item -> select Generic -> three entry point fields open it
  • Settings -> Diagnostics -> Roles By servers tab
  • Settings -> Region -> C and U tab
  • Compute -> Ingrastructure -> VMs -> Select one vm and click on genealogy
  • Compute -> Infra -> Networking // Still old tree
  • Compute -> Infra -> VMs -> Select one -> Snapshot button
  • Services -> Catalogs -> Catalog Items -> Edit item -> Tenants tree

------------------------------------------------------------------------------------------------------------------------------------
@miq-bot add_reviewer @skateman
@miq-bot add_reviewer @Hyperkid123
@miq-bot add_reviewer @himdel
@martinpovolny

@miq-bot add_label hammer/no
@miq-bot add_label react
@miq-bot add_label trees

@brumik brumik changed the title []WIP] Convert trees to react [WIP] Convert trees to react Nov 14, 2019
@miq-bot miq-bot added the wip label Nov 14, 2019
@brumik
Copy link
Author

brumik commented Dec 2, 2019

Todo

  • Services -> Catalogs -> Catalog Items and try to add a new item. If you select Generic, there will be three entry point fields available. The tree is not scrollable, so pressing the apply button can be challenging (has to close most of the nodes). - Entry point tree pop-up is not scrollable #6494
  • ^^ Same tree -> needs lazy load implementation
  • Compute -> Infra -> VMs -> Compare VM's add option: hierarchical check
  • Settings -> Region -> CU Collection tab -> Check all button not working yet
  • Option post-check is not required -> hierarchical tree always needs it.
    • Tenants tree: only post check not hierarchical (tree_checks)
    • Compare sections tree: hierarchical but no post check
  • Settings -> Access Control -> Roles/Edit Roles: When displaying add disable: true to the node states. Also, add hierarchical check and post-check.
  • Settings -> Diagnostics -> Roles By servers tab: Dim nodes by adding the class to the node and defining the class

@brumik brumik requested a review from martinpovolny as a code owner December 5, 2019 11:01
@skateman
Copy link
Member

Catalog tree styling should be fixed in #6511

@brumik brumik changed the title [WIP] Convert trees to react Convert trees to react Mar 12, 2020
@miq-bot miq-bot removed the wip label Mar 12, 2020
@skateman
Copy link
Member

@ZitaNemeckova could you please test this?

Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

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

The way we are handling the onclick/oncheck for each tree is a disaster. Of course you're not the one who's responsible for it, so I think we can keep it this way until I come up with something better. But there are a few nitpicks.

app/assets/javascripts/miq_tree.js Outdated Show resolved Hide resolved
app/assets/javascripts/miq_tree.js Outdated Show resolved Hide resolved
app/assets/javascripts/miq_tree.js Outdated Show resolved Hide resolved
app/assets/javascripts/miq_tree.js Outdated Show resolved Hide resolved
app/javascript/components/tree-view/reducers/basics.js Outdated Show resolved Hide resolved
app/javascript/components/tree-view/reducers/oncheck.js Outdated Show resolved Hide resolved
app/javascript/components/tree-view/reducers/oncheck.js Outdated Show resolved Hide resolved
@miq-bot
Copy link
Member

miq-bot commented Apr 29, 2020

Checked commits https://github.com/brumik/manageiq-ui-classic/compare/da20191813c2fc084a1fa0d82c131a270a4f76b6~...73ee7e1ce170f62f0b981593d4610d41361248c4 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
10 files checked, 2 offenses detected

app/views/miq_policy/_alert_profile_assign.html.haml

  • ⚠️ - Line 38 - Avoid using instance variables in partials views

app/views/shared/_tree.html.haml

  • ⚠️ - Line 4 - Layout/TrailingBlankLines: Final newline missing.

@brumik brumik requested a review from himdel May 1, 2020 07:15
@himdel himdel closed this May 20, 2020
@himdel himdel reopened this May 20, 2020
@himdel
Copy link
Contributor

himdel commented May 20, 2020

Merging, everything seems to work, no major issues :)

@brumik I do see 2 very minor issues, but we can solve those in a separate PR:

Group edit - assigned filters:

.indent-0 and .indent-0.no-open-button have inconsistent checkbox and icon alignments:

20200520191008

The same happens when adding an ansible playbook catalog item:

20200520191443


VM Snapshot - clicking an active item deactivates it:

20200520191307 mp4

on master, there's no deactivation (and no request when already active).

@himdel himdel merged commit e4e1412 into ManageIQ:master May 20, 2020
@himdel
Copy link
Contributor

himdel commented May 20, 2020

So, the indent issue:

from generateIndentCSS:
.indent-0.no-open-button gets 1em + 4px padding,
.indent-0 gets 0em padding,

=> the expand chevron total width would have to be 1em + 4px for this to work
and if we were only using wooden tree css, it would be almost right, the icon has min-width of 1em + right padding of 4px. (Almost because the actual width is set to 1.3em by .fa-fw, and padding is part of width in border-box.)

But also react-ui-components wooden-tree css overrides the icon, setting padding: 2px 1px 2px 2px, margin-right: 4px, width: 1em and box-sizing: content-box
converted to border-box, that translates to width: calc(1em + 3px), so the actual indentSize is 1em + 7px.


@brumik should we try to keep the spacing upstream?

If so, it needs to set a specific width for the chevron icon (not necesarily the other icons), and use that width+margin in the computation, something like...

diff --git a/src/components/Tree.tsx b/src/components/Tree.tsx
index 474500f..d977bd3 100644
--- react-wooden-tree/src/components/Tree.tsx
+++ react-wooden-tree/src/components/Tree.tsx
@@ -333,11 +333,18 @@ export class Tree extends React.PureComponent<TreeProps, {}> {
      */
     private static generateIndentCSS(depth: number): string {
         let cssRules: string = '';
-        let indentSize = 1;
+        let indentSize = '1em';
+        let chevronSize = '1em + 7px';
+
         for (let i = 0; i < depth; i++) {
             cssRules += `
-            .react-tree-view .indent-${i}{padding-left:${indentSize * i}em}
-            .react-tree-view .indent-${i}.no-open-button{padding-left:calc(${indentSize * i + indentSize}em + 4px)}`;
+            .react-tree-view .indent-${i} {
+                padding-left: calc(${indentSize} * ${i});
+            }
+            .react-tree-view .indent-${i}.no-open-button {
+                padding-left: calc(${indentSize} * ${i} + ${chevronSize});
+            }
+            `;
         }
         return cssRules;
     }

(Well, or just .icon { width: 1em; margin-right: 4px } to fit the existing code, but maybe the chevron, the checkbox, and the actual icon should all get their own css class for independent sizing.)

Or, if not, there should probably be a way to disable the indent css generation,
and we can do something like...

diff --git a/src/wooden-tree/index.scss b/src/wooden-tree/index.scss
index 7a364ea..b26be41 100644
--- react-ui-components/src/wooden-tree/index.scss
+++ react-ui-components/src/wooden-tree/index.scss
@@ -3,13 +3,12 @@
     padding-left: 0;
   }
 
-  li :first-child {
+  li > :first-child {
     display: inline-block;
   }
 
   i.icon {
-    width: 1em;
-    box-sizing: content-box;
+    width: calc(1em + 3px);
     padding: 2px;
     padding-right: 1px;
     margin-right: 4px;
@@ -23,4 +22,15 @@
     background-color: #349ad3;
     color: #FFFFFF;
   }
+
+  $indentSize: "1em";
+  $chevronSize: "1em + 7px";
+  @for $i from 0 through 5 {
+    .indent-#{$i} {
+      padding-left: calc(#{$indentSize} * #{$i}) !important;
+      &.no-open-button {
+        padding-left: calc(#{$indentSize} * #{$i} + #{$chevronSize}) !important;
+      }
+    }
+  }
 }

(!important only because the generated css now overrides anything of ours)


With the react-ui-components fix:

fixed

@brumik brumik deleted the convert-tree-to-react branch May 21, 2020 12:57
@brumik
Copy link
Author

brumik commented May 21, 2020

I am willing to make these changes to the component itself - it looks like the more reasonable solution. Also if you can propose a better CSS behaviour I am open: I know that generating and injecting CSS right under the tree (multiple times if there are multiple trees) is not perfect, but it was the best solution I could find.

@himdel
Copy link
Contributor

himdel commented May 21, 2020

Aah, great, agreed .. should I open a PR or do you want to do this? :)

As for the injection.. well, one possibility is to use invisible padder elements that can be naturally stacked, like having a .indent-level class that just has display: inline-block; width: 1em,
and using N .indent-level elements in a row, for each indent level. Such a thing might also be easier to override with custom css.

But I'm not really against the injection that much, if it can be relied upon. But maybe adding optional indentWidth, chevronWidth and generateCSS (bool) props would make it easier to fine tune the details.

@brumik
Copy link
Author

brumik commented May 21, 2020

I didn't want a stacker as it slows down the component by a huge margin and it was built for performance. If you can do it easily please create a PR there (don't have it locally since the reinstall). I think it uses schematic release it is just not documented yet (fix(css): othermsg should generate and upload a new version to npm).

@himdel
Copy link
Contributor

himdel commented May 21, 2020

OK, step 1: brumik/react-wooden-tree#50 (released in react-wooden-tree 2.1.5)
And I'll update react-ui-components & ui-classic to use it and remove css we no longer need.
(=> ManageIQ/react-ui-components#180 (released in @manageiq/react-ui-components 0.11.71))

@himdel
Copy link
Contributor

himdel commented Jun 11, 2020

Editing Role - the rbac tree no longer sends any miq_observe notifications - so form buttons no longer work. (found by @h-kataria)
(It also means you can't edit features at all, since the changes won't get saved.)

@himdel
Copy link
Contributor

himdel commented Jun 11, 2020

^ caused by 87f53cc

reducers gets called with oncheck undefined.
Without the change, the reducers work.

@skateman
Copy link
Member

skateman commented Jun 11, 2020

Just asked @brumik, he'll have a few hours to look at it tomorrow. It'll be a nice recharge 😉

@skateman
Copy link
Member

skateman commented Jun 12, 2020

The problem is that the tree name in edit and non-edit mode is the same, so first the tree is initialized without a reducer for oncheck and then re-rendered and not initialized with the right oncheck reducer. I guess the safest solution is to rename the non-edit tree as it has less moving parts.

#7127

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants