LibrarieJS UI changes by ramramps · Pull Request #109 · DynamoDS/librarie.js

ramramps · 2017-12-06T18:21:38Z

This PR is for LibrarieJS UI Changes.

Introduced "coregroup" for elements defined in Layoutspec.
Introduced "class" for elements defined in Layoutspec
Removed arrows for "coregroup"
Auto expand "coregroup" elements.
New icons

FYI Changes are made after review from @Racel

change group to coregroup in layoutspec add a new class for arrows in category

ramramps · 2017-12-08T16:07:55Z

src/components/LibraryItem.tsx

+                arrowIcon = require("../resources/ui/indent-arrow-down-wo-lines.svg");
+            }
+
+            return <img className={"CategoryArrow"} src={arrowIcon} onError={this.onImageLoadFail} />;


for arrow alignment on category

mjkkirschner · 2017-12-08T16:25:44Z

src/components/LibraryItem.tsx

        this.setState({ expanded: !currentlyExpanded });

+        //auto expand the coregroup elements
+        if(this.props.data.itemType === "category" ) {


does this work for nested categories?

No. this is only for the top level categories.

@Racel curious if this case was considered?

@mjkkirschner Only needed for top level categories.

mjkkirschner · 2017-12-08T17:41:26Z

docs/LayoutSpecs.json

              "text": "Color",
              "iconUrl": "",
-              "elementType": "group",
+              "elementType": "class",


so when does class get used? When should it be used opposed to group? Can you add these types to the documentation files? / wiki?

Do we ever generate classes or coreGroups when importing some other library which is not defined in the layout spec?

@ramramps also, so you will need to update the layoutspec in revit and dynamo as a consequence of this PR as well to get the same behavior right?

First, I understand this work is being done to accomplish the particular design requirements.
I think it would be useful for someone to enumerate the rules that are now encoded in this repo.
I could be wrong, but they seem pretty complicated to me.

I will take a shot:

Top Level categories display Icons

groups or classes do not display icons

Nested categories do not expand when their parent categories are clicked on.

Top Level categories expand their child groups when clicked on.

other stuff?

I do not think we will be able to maintain this matrix of interactions without properly documenting it and the new classes/ groups, etc...

This is not a criticism of the implementation here. I am fine with this approach, but would be happier if we did not need to introduce new types to accomplish this, just apply the minimal logic required to existing types throughout the library with no special logic for items in the layout spec - remember we'll need another giant layout spec for TS.

at a minimum let us please document these rules thoroughly.

@mjkkirschner Thanks for the comments.

Dynamo and Revit layoutspec needs a change.

This is definitely to get that particular design. This was easy in WPF. But in librarie.js, everything is dependent on elementType. I don't have a choice other than adding a new one.

Auto expand - as per the task, it is only for top level. Per my understanding from layoutspec, this is the order after the changesection-category-coregroup-group-none. All the nested category gets group by default.

Before the change, the order is section-category-group-none. So, there is no clear distinction between the top level and nested categories. Well, we can leave it this way too. Without introducing this new type, everything is group and no way to get this design based on other data. The other approach is to use a config file and white list the top level element.

I am afraid that if layoutspec is auto generated, then achieving this design will get complicated and requires careful look of how it is made today.
6.I will add a wiki page with rules per my understanding.

Also, I will rename coregroup to groups. So, the top level has groups of group.

@ramramps thanks for the detailed response - have you added the wiki page yet (6) on your list?

not yet. the rules are simplified now. no big change required in layoutspec. I will start the wiki , and let @Racel add the rules. She knows better than me.

mjkkirschner · 2017-12-08T18:08:24Z

@ramramps looks good implementation wise - some questions about other approaches not requiring new types, but LGTM if you have thought about that already.

ramramps · 2017-12-10T03:10:24Z

@mjkkirschner @Racel After giving it another shot, there is another way to achieve it.
No updates to layoutspec is required. group to coregroup is changed on the fly now.
So, no major updates required for layoutspec in revit or dynamo.
There is no other way for class update. that has to be made in layoutspec.

@Racel Let me know where and when to update class. I mean some kind of rulebook. I will update the wiki or readme section.

ramramps · 2017-12-10T03:11:21Z

src/LibraryUtilities.ts

+            child.childElements.forEach((grp: LayoutElement) => {
+                if(grp.elementType != "class") {
+                    grp.elementType = "coregroup";
+                }


I could have used underscore, but I find this way more explicit for someone to understand what we are doing (underscore does this implicit).

cool - looks good, can you add just a bit more description to the summary of the behavior - something like: changes categories topLevel children itemType from groups to coreGroups.... or something like that.

ramramps · 2017-12-10T03:12:34Z

__tests__/UITests.tsx

    }, 500);
  });
+
+  it("coregroup items should auto expand", function () {


this test covers the major part.

mjkkirschner · 2017-12-11T02:55:14Z

__tests__/Snapshottest/__snapshots__/UIOutputComparisonTests.tsx.snap

        onError={[Function]}
        src="test-file-stub"
      />
+      <img


so I think you might need to update the tests file after you merge with master as I think this will undo what Keith added.

ah..I think I discarded that change. Will check the master file.

mjkkirschner · 2017-12-11T03:01:17Z

src/LibraryUtilities.ts


 export function search(text: string, item: ItemData) {
-    if (item.itemType !== "group") {
+    if (item.itemType !== "group" && item.itemType !== "coregroup") {


should we also ignore class during search?

I don't think so. the idea is to change to class and use the search

mjkkirschner · 2017-12-11T03:02:31Z

@ramramps thanks for taking another look at this - just a few more comments. Then LGTM.

ramramps · 2017-12-11T11:42:42Z

@Racel https://wiki.autodesk.com/display/GEN/Rule+book
Here is a very basic one. We can update / remove contents from this.

ramramps · 2017-12-11T11:46:46Z

@Racel @mjkkirschner Let me know whether I can merge this code.
I will send PR for Dynamo after that.

Racel · 2017-12-11T12:56:55Z

LGTM. Lets merge and see how it goes...

Racel · 2017-12-11T13:55:24Z

@ramramps - Before you merge, can you remove the arrow from the very top level category? I think it is too much...

QilongTang · 2017-12-11T15:05:39Z

@ramramps We will just merge this one and create different defect for UI improment. Thanks

alfarok · 2017-12-11T20:20:47Z

src/components/LibraryItem.tsx

+        this.props.data.itemType !== "section" && 
+        this.props.data.itemType !== "coregroup" &&
+        this.props.data.itemType !== "class" &&
+        this.props.data.itemType == "none") {


should be this.props.data.itemType !== "none"

QilongTang · 2017-12-11T20:37:06Z

src/resources/icons/arrow-pointing-to-right.svg

@@ -0,0 +1,38 @@
+<?xml version="1.0" encoding="iso-8859-1"?>


This icon does not seem referenced anywhere!

QilongTang · 2017-12-11T20:38:32Z

src/resources/icons/library-action-old.svg

@@ -0,0 +1,22 @@
+<?xml version="1.0" encoding="utf-8"?>


This icon does not seem referenced anywhere!

QilongTang · 2017-12-11T21:07:08Z

src/components/LibraryItem.tsx

+        this.props.data.itemType !== "section" && 
+        this.props.data.itemType !== "coregroup" &&
+        this.props.data.itemType !== "class" &&
+        this.props.data.itemType == "none") {


This should be !== will address in a different PR

QilongTang · 2017-12-11T21:40:54Z

src/resources/UI/indent-arrow-right-wo-lines.svg

+<style type="text/css">
+	.st0{fill:#808080;}
+</style>
+<title>indent-arrow-right</title>


You may want to update titles as well

ramramps added 5 commits December 6, 2017 23:50

Fix for Librarie UI

d60ce5b

change to coregroup in layoutspec

28084d0

change group to coregroup in layoutspec add a new class for arrows in category

auto expand group

0741f36

unit test

19a6dc6

remove icon for class

2928256

ramramps changed the title ~~[WIP] LibrarieJS UI changes~~ LibrarieJS UI changes Dec 8, 2017

ramramps commented Dec 8, 2017

View reviewed changes

comment

ab2600c

mjkkirschner reviewed Dec 8, 2017

View reviewed changes

Updates without changing layoutspec

92ff4ed

ramramps commented Dec 10, 2017

View reviewed changes

ramramps added 2 commits December 10, 2017 08:43

test update

49d6d55

merge with master

561c83c

mjkkirschner reviewed Dec 11, 2017

View reviewed changes

Comments

5db5777

update to tests

48fd78e

QilongTang merged commit 6131cfb into master Dec 11, 2017

QilongTang deleted the LibrarieJS-update branch December 11, 2017 15:56

QilongTang mentioned this pull request Dec 11, 2017

Update LibrarieJs DynamoDS/Dynamo#8392

Merged

7 tasks

alfarok reviewed Dec 11, 2017

View reviewed changes

QilongTang reviewed Dec 11, 2017

View reviewed changes

QilongTang mentioned this pull request Dec 11, 2017

Fix For Librarie Icon #110

Merged

QilongTang reviewed Dec 11, 2017

View reviewed changes

ramramps mentioned this pull request Mar 8, 2018

Update elementTypes for watch, color, and color range DynamoDS/Dynamo#8630

Merged

7 tasks

Conversation

ramramps commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramramps Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramramps Dec 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramramps Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Dec 8, 2017

Uh oh!

ramramps commented Dec 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ramramps commented Dec 11, 2017

Uh oh!

ramramps commented Dec 11, 2017

Uh oh!

Racel commented Dec 11, 2017

Uh oh!

Racel commented Dec 11, 2017

Uh oh!

QilongTang commented Dec 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramramps commented Dec 6, 2017 •

edited

Loading

ramramps Dec 8, 2017 •

edited

Loading

mjkkirschner Dec 8, 2017 •

edited

Loading

mjkkirschner Dec 8, 2017 •

edited

Loading

ramramps Dec 9, 2017 •

edited

Loading

ramramps Dec 11, 2017 •

edited

Loading

ramramps commented Dec 10, 2017 •

edited

Loading

mjkkirschner Dec 11, 2017 •

edited

Loading

mjkkirschner commented Dec 11, 2017 •

edited

Loading