feat: refactor side navigation #380

InnaAtanasova · 2019-10-21T18:42:46Z

Related Issue

Closes #207

Description

added a new (INTERNAL for the lib) reusable component called Nested list which will allow multiple levels of nesting (up to 10)
use the Nested List component in Side Navigation component
refactor the Side Nav component to match the latest specs
The Side nav now has 2 sections - Main Section and Utility Section. Each of these sections has a Nested List for displaying the navigation items.

BREAKING CHANGES

Removed classes: fd-side-nav__list, fd-side-nav__item, fd-side-nav__link, fd-side-nav__group, fd-side-nav__title, fd-side-nav__sublist, fd-side-nav__subitem, fd-side-nav__sublink, fd-side-nav__icon, fd-side-nav--icons

netlify · 2019-10-21T18:45:45Z

Deploy preview for fundamental-styles ready!

Built with commit 873711f

https://deploy-preview-380--fundamental-styles.netlify.com

bcullman · 2019-10-22T15:36:23Z

Should this even be needed?

With proper CSS, any li that is a direct child of a li ul can be set to indented - no need for an indented class or a collection of classes to name what level deep your are

bcullman · 2019-10-22T16:14:07Z

note that plain old html indents children

<html>
<body>
    <ul>
        <li>1</li>
        <li>2</li>
        <li>3
            <ul>
                <li>a</li>
                <li>b</li>
                <li>c
                    <ul>
                        <li>i</li>
                        <li>ii</li>
                        <li>iii</li>
                    </ul>
                </li>
            </ul>
        </li>
    </ul>
</body>
</html>

markpalfreeman

@greg-a-smith how does this jive with the work you did on standard List?

Are we trying to only create fundamental-styles "components" that match to UI5 controls? If so, I'm not sure we need a NestedList as a separate one unless we're actually planning to reuse it somewhere else besides SideNavigation.

markpalfreeman · 2019-10-22T16:37:56Z

src/nested-list.scss

+  speak: none;
+}
+
+@mixin fd-flex-center {


This would probably better live in a shared utilities folder, as it's not particular to this file.

markpalfreeman · 2019-10-22T16:41:30Z

src/nested-list.scss

+
+      .#{$block}__link,
+      .#{$block}__content {
+        padding-left: $paddingLeft;


As @bcullman mentioned previously, I don't think we want to be determining the "level" of nesting and calculating padding for each form the outer-most point... wouldn't it be preferred to instead put padding on each "parent" ul (with some modifier like .fd-list--expandable) and let the children indent themselves (recursively)? It would eliminate the need for this custom math.

As a visible example from UI5:

Yes, in most of the cases this would be the approach, but not in this one. If you add padding to the ul element then the selected and hover styling of the subitems (children) wouldn't be correct. The background color and the underline border should go to the ends of the Navigation list.
Here is how it looks now:

Here is the case when we add padding to the ul element and don't do the calculations

Makes sense, I didn't see the hover/active states previously.

I still don't know that we want the consumers of this to have to keep track of the "level" of indentation in their markup to supply the correct class.

Seems like using parent/child relationship selectors allows consumers to just apply classes more generically, and the CSS is smart enough to style itself, like:

.list .list__item .list__item .list__item .list .list__item .list__item .list__item .list__item

InnaAtanasova · 2019-10-22T17:38:28Z

Should this even be needed?

With proper CSS, any li that is a direct child of a li ul can be set to indented - no need for an indented class or a collection of classes to name what level deep your are

@bcullman , applying padding to the li adds indentation but doesn't have the correct styling for hover, is-selected and focus state.

droshev · 2019-10-22T17:53:31Z

to only create fundamental-styles "components" that match to UI5 controls? If so, I'm not sure we need a NestedList as a separate one unless we're actuall

@markpalfreeman It is not only needed for the side navigation and we are not first creating that. UI5 has different components too

InnaAtanasova · 2019-10-22T18:04:32Z

@greg-a-smith how does this jive with the work you did on standard List?

Are we trying to only create fundamental-styles "components" that match to UI5 controls? If so, I'm not sure we need a NestedList as a separate one unless we're actually planning to reuse it somewhere else besides SideNavigation.

@markpalfreeman , my approach at the beginning was to refactor the List component and achieve the nesting there. But it required too many changes. For starters, the current implementation uses flex for situating the icons and text. This causes problems when you add a ul inside the fd-list__item.
Second, you can "mix" items with and without icons, and the items without icons should have the same text alignment as the items with icons.
Third, the "after" icon (the arrow) should be dynamic - it should have different states when the child list is expanded and not-expanded. This can't be achieve with CSS and the current implementation of the List.
The list items should be links. The list item in List component is just an icon plus text, it doesn't have a link

markpalfreeman

If we go forward with Nested List, I think tit'd be the first time this library would be exporting a non-Fiori3 component and referencing it in public docs/navigation.

If this is something that needs to be shared in multiple places, my preference would be to use it as a sass mixin that gets brought into the components that need it, but the class names stay aligned to Fiori3 components so we have a 1:1 match.

Probably worth deciding as a team, but to me it gets confusing if we start creating "non-standard" components here—how do we keep track of which ones are Fiori3 and which ones are "helpers"?

markpalfreeman · 2019-10-22T22:20:19Z

src/nested-list.scss

+
+      .#{$block}__link,
+      .#{$block}__content {
+        padding-left: $paddingLeft;


I still don't know that we want the consumers of this to have to keep track of the "level" of indentation in their markup to supply the correct class.

Seems like using parent/child relationship selectors allows consumers to just apply classes more generically, and the CSS is smart enough to style itself, like:

.list .list__item .list__item .list__item .list .list__item .list__item .list__item .list__item

markpalfreeman

Thanks Inna! I have a few pending questions and would still prefer someone with more familiarity to chime in if possible 😄

markpalfreeman · 2019-10-25T18:13:37Z

src/_nested-list.scss

+
+$block: #{$fd-namespace}-nested-list;
+
+@mixin fd-side-nav-arrow {


nit: since this is nested-list.scss, this is probably better named fd-nested-list-arrow to not couple to the "side-nav" concept

markpalfreeman · 2019-10-25T18:14:43Z

src/_nested-list.scss

+    }
+  }
+
+  &[aria-hidden="true"] {


I think we want an additional .is-hidden class to not fully rely on aria attributes for styles?

so I think we never fully decided how we want to handle all the aria-selectors. We've got it implemented many ways across components with no real standard. We should figure this out in general, but maybe not here.

markpalfreeman · 2019-10-25T18:41:40Z

src/_nested-list.scss

+  list-style: none;
+  width: 100%;
+
+  @for $i from 2 through 10 {


I'm not sure if my previous comment got lost or if I forgot to post it, but do we want consuming libraries to have to calculate .level-# for classes?

I love that this is a mixin, but it seems like a preferred hierarchy would just be something like this, and the styles would be ready to support all tiers of parent/child relationship (and padding size) up to a certain level.

Thoughts from the rest of the team? (pseudo-code)

.list .list__item .list__item .list .list__item .list__item ...

Brad did rightly mention, though, that the tradeoff for this is making really deep parent-child selectors in the CSS 😕

.list > .list__item > list > .list__item {} etc

I'm not sure which is best

@markpalfreeman what would you suggest?

We can probably just leave it like this; both ways have tradeoffs.

Primary issue addressed, new set of questions

greg-a-smith

Appears that a few things aren't self-contained for styling yet (see screenshot).

InnaAtanasova · 2019-10-28T18:02:13Z

Appears that a few things aren't self-contained for styling yet (see screenshot).

Hi @greg-a-smith ,
I added a reset for the TEST list items.

greg-a-smith · 2019-10-29T19:48:06Z

test/resources/layout.css

+  margin: 0;
+  border: 0;
+  list-style: none;
+}


These shouldn't be necessary, should they? The playground is supposed to demonstrate the component and its styles. The "test" classes are for extraneous components included within another component (e.g. buttons in an action bar or modal, icons in a panel, etc.)

greg-a-smith · 2019-10-30T03:45:59Z

test/templates/side-nav/index.njk

-    )}}
-    {% endset %}
-    {{ format(example) }}
+<h2>Nested List Without Icons</h2>


All these nested list examples should be in their own page: test/templates/nested-list/index.njk. This file should only have the examples that show the fd-side-nav* classes.

greg-a-smith

Looks good. 🚢

InnaAtanasova requested review from markpalfreeman, droshev, bcullman, greg-a-smith, JKMarkowski, jbadan and hertweckhr1 October 21, 2019 18:42

droshev added the Enhancement New feature or request label Oct 21, 2019

droshev added this to the sprint 22 - Gendry milestone Oct 21, 2019

droshev approved these changes Oct 22, 2019

View reviewed changes

markpalfreeman previously requested changes Oct 22, 2019

View reviewed changes

droshev requested a review from markpalfreeman October 22, 2019 22:08

markpalfreeman reviewed Oct 22, 2019

View reviewed changes

InnaAtanasova requested a review from markpalfreeman October 24, 2019 18:06

InnaAtanasova changed the title ~~feat: new component nested list, refactor side navigation~~ feat: refactor side navigation Oct 24, 2019

InnaAtanasova added 5 commits October 24, 2019 14:23

add new component Nested List, refactor Side Nav

54f7f4d

add files for the test playground

321a0f4

add visual tests

b11c7b2

add visual tests for the menu

e3fda84

documentation improvements

4528e27

removed nested list from public components

deb0574

InnaAtanasova force-pushed the feat/nested-list__side-nav branch from 7fd5411 to deb0574 Compare October 24, 2019 18:29

remove tests for nested-list

a90b601

markpalfreeman reviewed Oct 25, 2019

View reviewed changes

greg-a-smith suggested changes Oct 25, 2019

View reviewed changes

add reset for the test sidenav items

be1d218

InnaAtanasova requested a review from greg-a-smith October 28, 2019 18:26

greg-a-smith suggested changes Oct 29, 2019

View reviewed changes

add tests for nested list in side nav

8782de5

greg-a-smith reviewed Oct 30, 2019

View reviewed changes

InnaAtanasova added 3 commits October 30, 2019 09:03

add visual regression tests for side nav and nested list

c06fb2a

separate nested list test template from side nav

0ff9142

update visual tests

d92d662

InnaAtanasova requested a review from greg-a-smith October 30, 2019 14:05

add minor fixes and clean up

873711f

greg-a-smith approved these changes Oct 30, 2019

View reviewed changes

InnaAtanasova merged commit 9ab0628 into master Oct 30, 2019

InnaAtanasova deleted the feat/nested-list__side-nav branch October 30, 2019 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: refactor side navigation #380

feat: refactor side navigation #380

InnaAtanasova commented Oct 21, 2019 •

edited

Loading

netlify bot commented Oct 21, 2019 •

edited

Loading

bcullman commented Oct 22, 2019

bcullman commented Oct 22, 2019 •

edited

Loading

markpalfreeman left a comment

markpalfreeman Oct 22, 2019

InnaAtanasova Oct 24, 2019

markpalfreeman Oct 22, 2019 •

edited

Loading

InnaAtanasova Oct 22, 2019 •

edited

Loading

markpalfreeman Oct 22, 2019

markpalfreeman Oct 22, 2019

InnaAtanasova commented Oct 22, 2019

droshev commented Oct 22, 2019 •

edited

Loading

InnaAtanasova commented Oct 22, 2019 •

edited

Loading

markpalfreeman left a comment

markpalfreeman Oct 22, 2019

markpalfreeman left a comment

markpalfreeman Oct 25, 2019

markpalfreeman Oct 25, 2019

jbadan Oct 25, 2019

markpalfreeman Oct 25, 2019

markpalfreeman Oct 25, 2019

droshev Oct 25, 2019

markpalfreeman Oct 25, 2019

greg-a-smith left a comment

InnaAtanasova commented Oct 28, 2019

greg-a-smith Oct 29, 2019

greg-a-smith Oct 30, 2019

greg-a-smith left a comment


		$block: #{$fd-namespace}-nested-list;

		@mixin fd-side-nav-arrow {

feat: refactor side navigation #380

feat: refactor side navigation #380

Conversation

InnaAtanasova commented Oct 21, 2019 • edited Loading

Related Issue

Description

BREAKING CHANGES

netlify bot commented Oct 21, 2019 • edited Loading

bcullman commented Oct 22, 2019

bcullman commented Oct 22, 2019 • edited Loading

markpalfreeman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markpalfreeman Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

InnaAtanasova Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InnaAtanasova commented Oct 22, 2019

droshev commented Oct 22, 2019 • edited Loading

InnaAtanasova commented Oct 22, 2019 • edited Loading

markpalfreeman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markpalfreeman 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-a-smith left a comment

Choose a reason for hiding this comment

InnaAtanasova commented Oct 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-a-smith left a comment

Choose a reason for hiding this comment

InnaAtanasova commented Oct 21, 2019 •

edited

Loading

netlify bot commented Oct 21, 2019 •

edited

Loading

bcullman commented Oct 22, 2019 •

edited

Loading

markpalfreeman Oct 22, 2019 •

edited

Loading

InnaAtanasova Oct 22, 2019 •

edited

Loading

droshev commented Oct 22, 2019 •

edited

Loading

InnaAtanasova commented Oct 22, 2019 •

edited

Loading