Define SideNav items with a tree structure #141

cjcenizal · 2017-11-12T04:20:46Z

While I was playing with this, the static behavior of the carets felt weird. I'd click on an item to open it, and the caret felt redundant once it was opened. I wonder if we even need the caret? Is it there to communicate to users that something expands? If so, do we need to communicate that?

To-do:

Document isInline prop on flex components
Discuss "open parent" visual style with designers
Fix left padding of individual items so that focus state doesn't look odd

bevacqua · 2017-11-12T21:15:42Z

Would be nice if we started pasting screenshots of the PR to get a glimpse of what's implemented/changed. Think you could do that here? 😅

cjcenizal · 2017-11-13T06:27:52Z

@bevacqua Yep! I was going to add a gif once this was ready for review. Sorry, I should have added a WIP tag.

cjcenizal · 2017-11-14T06:10:39Z

@snide @gjones @Adrian-J @formgeist @bevacqua This is ready for your review! I suggest checking out this branch and playing with it in the browser to get a feel for the interaction.

The side nav in the docs themselves is also worth trying out because, due to a quirk in the implementation, the subsections of each page aren't registered until after that page has been visited. Which means that each section initially doesn't show a caret, but does show a caret after you visit that page. To be honest, I didn't miss the caret. What do y'all think?

Adrian-J · 2017-11-14T08:39:08Z

@cjcenizal looks great. only comment, can the expanded version push the rest down instead of up?

formgeist · 2017-11-14T08:45:46Z

Does it have to be font-weight: 500; on selected state? It feels a little weird to me when the letter-spacing jumps around because it adds additional weight for each click. I tested it out locally I thought it was more "calm" when it was just regular all the way.

cjcenizal · 2017-11-14T15:24:42Z

Thanks @Adrian-J that "pushing up" is a side-effect because this example is at the bottom of the page. So it's making the page taller and Chrome will "stick" you to the bottom of the page as the page expands. So long story short... yes, it's already pushing the rest down as you would expect.

@formgeist I'll leave that one to @snide.

gjones · 2017-11-14T16:21:53Z

Something does feel a little off to me with that caret, I think it's because I would expect it to be the arrowRight icon and then transition to arrowDown when the sub menu is expanded. I also think that it should live maybe 4 or 6 pixels to the right of the word rather than as far away as it is now. Sorry I haven't taken too long to think this over, these are just my immediate thoughts, feel free to disregard.

So something like below (excuse the hasty, gross designs)

becomes

Also, I should add, I wouldn't be opposed to the carets being light grey and only appearing on hover.

Adrian-J · 2017-11-14T16:24:23Z

@cjcenizal makes sense. One more thing I haven't noticed before, I think the 2nd level vertical line should be left-aligned with the 1st level title. Looks cleaner that way.

cjcenizal · 2017-11-14T16:27:13Z

@Adrian-J It's funny you mention that because I actually had the same thought and tried it out:

When I looked at it originally it felt weird to me, but now looking at it again I kind of like it. I'll let you and Dave hash it out.

Adrian-J · 2017-11-14T17:44:04Z

@cjcenizal I like this version much better. Great job!

snide

Minor stuff. Feel free to merge whenever.

I do think it's really weird to see the carets in the docs only after you've clicked them. Can fix later, but if that happened in a prod scenario I'd think it was kind of busted.

snide · 2017-11-14T17:20:32Z

src/components/side_nav/_side_nav_item.scss

+    margin-bottom: $euiSizeS;
+    padding: 0;
+    padding-left: $euiSizeS; /* 1 */
+    margin-left: -$euiSizeS; /* 1 */


This trick messes up your focus state width. Use a calc width calc(100% + #{$euiSizeS}) and add your negative margin back to adjust it. Essentially you want the right hand line to carry straight across the entire side.

snide · 2017-11-14T17:34:56Z

src-docs/src/views/flex/use_span.js

+
+export default () => (
+  <EuiFlexGroup useSpan>
+    <EuiFlexItem useSpan>This a span within a span</EuiFlexItem>


Any time you'd mix spans and divs? If not, should this just be on EuiFlexGroup and not the items itself? Not a blocker, just if it's easy it's probably easier to consume.

Maybe give a better example of why you'd want to use spans? I assume it's because you want to sit within a button rather than KeyboardAccessible? Maybe show the example inside of a button then to illustrate that?

Any time you'd mix spans and divs?

Good question, but I'm not sure to be honest. Since I can't say one way or the other I think I should assume there's a good use case I'm just not aware of. We can find a way to lock it down later if people find this un-ergonomic.

Maybe give a better example of why you'd want to use spans?

Good call, I'll put it inside a button to make this clearer.

Yep, makes sense.

snide · 2017-11-14T17:46:15Z

src/components/side_nav/_side_nav_item.scss

  }

-  &.euiSideNavItem--parent.euiSideNavItem-isSelected {
-    color: $euiColorFullShade;
+  &.euiSideNavItemButton-isOpenable {
    background: svg-load('../src/components/icon/assets/arrow_down.svg') center right no-repeat;


If possible, I'd instead use EuiIcon instead of this hacky CSS solution. That way it can be colored (it should probably be subdued). Right now this breaks in dark mode because it's hard coded.

Don't want to hamper your PR, so if you merge it as is, just make an issue to cover it. We can figure it out later.

snide · 2017-11-14T22:24:59Z

@cjcenizal Don't forget to update the changelog before you merge this.

zinckiwi · 2017-11-15T07:51:02Z

src-docs/src/components/guide_page/guide_page_chrome.js

+          return {
+            ...section,
+          };
+        });


tiny stylistic proposal:

=> ({ ...section })

zinckiwi · 2017-11-15T07:53:25Z

src-docs/src/components/guide_page/guide_page_chrome.js

+            name: subSection.name,
+            onClick: this.onClickLink.bind(this, subSection.id),
+          }));
+        }


#nicohat -- could lose the mutable var by turning this into a function that takes subSections and guards on it being falsy

zinckiwi · 2017-11-15T07:54:30Z

src-docs/src/components/guide_page/guide_page_chrome.js

+            name: subSection.name,
+            onClick: this.onClickLink.bind(this, subSection.id),
+          }));
+        }


(and benefit from it here as well)

zinckiwi · 2017-11-15T07:58:00Z

src/components/flex/flex_group.js

+  justifyContent,
+  useSpan,
+  ...rest,
+}) => {


Rather than useSpan which seems overly prescriptive, could I suggest component: Component = 'div' and return <Component ... />? Could easily propType it to oneOf(['div', 'span']) to lock it down to those two.

Edit: to clarify, usage would be <FlexGroup component='span' /> instead of <FlexGroup useSpan />

- Automatically determine open state based on selected item. - Add support for href and onClick. - Allow root items to be collapsed.

…cus state.

cjcenizal · 2017-11-16T19:38:26Z

@snide I updated the changelog, thanks for the reminder. @zinckiwi I addressed your comments, could you take another look?

zinckiwi · 2017-11-22T20:23:45Z

CHANGELOG.md

@@ -1,8 +1,13 @@
 # [`master`](https://github.com/elastic/eui/tree/master)

 - `<EuiFlexItem>` now accepts integers between 1 and 10 for the `grow` prop. [(#144)](https://github.com/elastic/eui/pull/144)
+- `<EuiFlexItem>` and `<EuiFlexGrow>` now accept a `component` prop which you can set to `span` or `div`. [(#141)](https://github.com/elastic/eui/pull/141)


Perhaps an indication as to the default (which therefore shouldn't be explicitly specified)?

zinckiwi · 2017-11-22T20:24:12Z

LGTM aside from one very minor comment about the changelog entry!

cjcenizal · 2017-11-22T20:32:59Z

Thanks @zinckiwi !

cjcenizal added work in progress and removed work in progress labels Nov 13, 2017

snide reviewed Nov 14, 2017

View reviewed changes

cjcenizal requested review from zinckiwi and pugnascotia November 14, 2017 18:22

snide added the breaking change label Nov 14, 2017

zinckiwi reviewed Nov 15, 2017

View reviewed changes

cjcenizal added 9 commits November 15, 2017 17:15

Define SideNav items with a tree data structure.

b14707c

- Automatically determine open state based on selected item. - Add support for href and onClick. - Allow root items to be collapsed.

Add support for inline FlexGroup and FlexItem.

ee810b5

Remove caret from open items.

771a13f

Remove unused code and update snapshots.

9ae03a4

Add padding around root and trunk buttons to create a less cramped fo…

083640a

…cus state.

Simplify modifier and state classes, and rename them for clarity.

5ba65a1

Rename isInline prop to useSpan and add docs example.

8e8cd93

Fix bug with underline color.

79b525a

Minor cleanup.

f35d8be

cjcenizal added 6 commits November 15, 2017 17:16

Clarify useSpan example by putting the items inside a button.

696ccf6

Use EuiIcon to place caret. Fix padding and width CSS.

d91fbed

Add tests.

ff190c1

Rename complicated example to complex. Update documentation.

fd163b2

Remove unused code and add renderSubSections method to GuidePageChrome.

105de7d

Replace useSpan prop with component prop.

0195fae

cjcenizal force-pushed the improvement/side-nav-interface branch from 75e0b1b to 0195fae Compare November 16, 2017 19:33

Update CHANGELOG.md.

3368bc3

zinckiwi reviewed Nov 22, 2017

View reviewed changes

Add note about default component prop.

0c144b0

cjcenizal merged commit 0c9b8ce into elastic:master Nov 22, 2017

cjcenizal deleted the improvement/side-nav-interface branch November 22, 2017 20:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define SideNav items with a tree structure #141

Define SideNav items with a tree structure #141

cjcenizal commented Nov 12, 2017 •

edited

Loading

bevacqua commented Nov 12, 2017

cjcenizal commented Nov 13, 2017

cjcenizal commented Nov 14, 2017 •

edited

Loading

Adrian-J commented Nov 14, 2017

formgeist commented Nov 14, 2017

cjcenizal commented Nov 14, 2017

gjones commented Nov 14, 2017 •

edited

Loading

Adrian-J commented Nov 14, 2017

cjcenizal commented Nov 14, 2017 •

edited

Loading

Adrian-J commented Nov 14, 2017

snide left a comment

snide Nov 14, 2017

snide Nov 14, 2017

cjcenizal Nov 14, 2017

snide Nov 14, 2017

snide Nov 14, 2017

snide commented Nov 14, 2017

zinckiwi Nov 15, 2017

zinckiwi Nov 15, 2017

zinckiwi Nov 15, 2017

zinckiwi Nov 15, 2017 •

edited

Loading

cjcenizal commented Nov 16, 2017

zinckiwi Nov 22, 2017

zinckiwi commented Nov 22, 2017

cjcenizal commented Nov 22, 2017

Define SideNav items with a tree structure #141

Define SideNav items with a tree structure #141

Conversation

cjcenizal commented Nov 12, 2017 • edited Loading

bevacqua commented Nov 12, 2017

cjcenizal commented Nov 13, 2017

cjcenizal commented Nov 14, 2017 • edited Loading

Adrian-J commented Nov 14, 2017

formgeist commented Nov 14, 2017

cjcenizal commented Nov 14, 2017

gjones commented Nov 14, 2017 • edited Loading

Adrian-J commented Nov 14, 2017

cjcenizal commented Nov 14, 2017 • edited Loading

Adrian-J commented Nov 14, 2017

snide 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snide commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zinckiwi Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

cjcenizal commented Nov 16, 2017

Choose a reason for hiding this comment

zinckiwi commented Nov 22, 2017

cjcenizal commented Nov 22, 2017

cjcenizal commented Nov 12, 2017 •

edited

Loading

cjcenizal commented Nov 14, 2017 •

edited

Loading

gjones commented Nov 14, 2017 •

edited

Loading

cjcenizal commented Nov 14, 2017 •

edited

Loading

zinckiwi Nov 15, 2017 •

edited

Loading