addition of nav-item component styles and style behavior #28

abu-seini-maf · 2024-11-17T12:14:28Z

@carlos-verdes @moalobaidat:
NOT For Merging, only review to align on progress and approach.

carlos-verdes · 2024-11-18T04:36:51Z

src/components/mf-nav-item/mf-nav-item.component.ts

+    const loopingBehavior = (passedNavItemParent: HTMLElement) => {
+      const navElementParent = passedNavItemParent;
+      const isMainNav =
+        navElementParent?.tagName?.toLocaleLowerCase() === 'mf-navigation' &&


Would you consider to rename from mf-navigation to mf-nav in a separate PR?

yes it should be renamed to mf-navigation, i'll be doing it once am back from vacation.
or someone else can pick up

You meant maf-nav right? Someone can pick up this if we are aligned with the name

carlos-verdes · 2024-11-18T04:39:09Z

src/components/mf-nav-item/mf-nav-item.component.ts

+  private getNavItemLevel(navItemParent: HTMLElement | null) {
+    let level = 0;
+
+    const loopingBehavior = (passedNavItemParent: HTMLElement) => {


I read an article that discourage this kind of lambda expressions as they can cause memory leaks.
This is just a comment to share this information, no need to update the code

For reference here you can read the article: on-js-closures-and-leaks

i needed a behavior that can distinguish each level of the navigation items, to be able to style it

In the article it recommends to avoid closures, that's what is the comment about :)

carlos-verdes

Put some comment we can discuss more later

carlos-verdes · 2024-11-18T04:42:19Z

src/components/mf-nav-item/mf-nav-item.styles.ts

+    font-family: var(--mafui-navigationItem-font-family-primary);
+    font-size: var(--mafui-navigationItem-font-size-primary);
+    font-weight: var(--mafui-navigationItem-font-weight-primary);
+    line-height: var(--mafui-navigationItem-line-height-primary);
+    letter-spacing: var(--mafui-navigationItem-letter-spacing-primary);
+    color: var(--mafui-navigationItem-font-color-primary);
+    background-color: var(--mafui-navigationItem-color-background-primary);
+    border-top: var(--mafui-navigationItem-border-width-top-primary)
+      var(--mafui-navigationItem-border-style-top-primary) var(--mafui-navigationItem-color-border-top-primary);
+    border-right: var(--mafui-navigationItem-border-width-right-primary)
+      var(--mafui-navigationItem-border-style-right-primary) var(--mafui-navigationItem-color-border-right-primary);
+    border-bottom: var(--mafui-navigationItem-border-width-bottom-primary)
+      var(--mafui-navigationItem-border-style-bottom-primary) var(--mafui-navigationItem-color-border-bottom-primary);
+    border-left: var(--mafui-navigationItem-border-width-left-primary)
+      var(--mafui-navigationItem-border-style-left-primary) var(--mafui-navigationItem-color-border-left-primary);


I would remove -primary on the first level, for example:

font-family: var(--mafui-navigationItem-font-family);

it seems fair, as its the main style for all navigation items.
but also sounds a bit counter intuitive, lets decide on this.
@moalobaidat

My point is every menu can can levels 1, 2 and 3 (we will discourage to use more than 3 levels) and we can have more than one menu.

Each menu variant should have it's own suffix like primary, secondary etc. and each menu level it's own suffix too

Thanks for the explanation, @carlos-verdes . I get your point about using clear suffixes for consistency, but I’m not sure removing -primary from font-family is the right move. The -primary suffix helps keep things clear by showing what the default style should be, especially as we scale and maintain the code.

Maybe instead of removing it, we could look at how we’re using these suffixes and make sure they’re not confusing or redundant.

Let’s discuss further to make sure we’re keeping things both clear and easy to maintain.

carlos-verdes · 2024-11-18T04:44:33Z

src/components/mf-nav-item/mf-nav-item.styles.ts

+  :host([level='level-1']) .nav-item {
+    font-family: var(--mafui-navigationItem-font-family-secondary);
+    font-size: var(--mafui-navigationItem-font-size-secondary);
+    font-weight: var(--mafui-navigationItem-font-weight-secondary);
+    line-height: var(--mafui-navigationItem-line-height-secondary);
+    letter-spacing: var(--mafui-navigationItem-letter-spacing-secondary);
+    color: var(--mafui-navigationItem-font-color-secondary);
+    background-color: var(--mafui-navigationItem-color-background-secondary);
+    border-top: var(--mafui-navigationItem-border-width-top-secondary)
+      var(--mafui-navigationItem-border-style-top-secondary) var(--mafui-navigationItem-color-border-top-secondary);
+    border-right: var(--mafui-navigationItem-border-width-right-secondary)
+      var(--mafui-navigationItem-border-style-right-secondary) var(--mafui-navigationItem-color-border-right-secondary);
+    border-bottom: var(--mafui-navigationItem-border-width-bottom-secondary)
+      var(--mafui-navigationItem-border-style-bottom-secondary)
+      var(--mafui-navigationItem-color-border-bottom-secondary);
+    border-left: var(--mafui-navigationItem-border-width-left-secondary)
+      var(--mafui-navigationItem-border-style-left-secondary) var(--mafui-navigationItem-color-border-left-secondary);
+  }


level-1 and -secondary is confusing to me, I would use level-1 for first parent and default css variables (following previous commit) and level-2 for secondary

Also I was thinking secondary is a variant and it may be useful to use it for a secondary menu (imagine a section with settings for example), so maybe is better to be explicit and use -level-2 as suffix:

:host([level='level-2']) .nav-item { font-family: var(--mafui-navigationItem-font-family-level-2);

LGTM, @moalobaidat ?

carlos-verdes · 2024-11-18T04:50:20Z

src/components/mf-nav-item/mf-nav-item.styles.ts

+    border-top: var(--mafui-navigationItem-border-width-top-tertiary)
+      var(--mafui-navigationItem-border-style-top-tertiary) var(--mafui-navigationItem-color-border-top-tertiary);
+    border-right: var(--mafui-navigationItem-border-width-right-tertiary)
+      var(--mafui-navigationItem-border-style-right-tertiary) var(--mafui-navigationItem-color-border-right-tertiary);
+    border-bottom: var(--mafui-navigationItem-border-width-bottom-tertiary)
+      var(--mafui-navigationItem-border-style-bottom-tertiary) var(--mafui-navigationItem-color-border-bottom-tertiary);
+    border-left: var(--mafui-navigationItem-border-width-left-tertiary)
+      var(--mafui-navigationItem-border-style-left-tertiary) var(--mafui-navigationItem-color-border-left-tertiary);


not sure about having 4 variables one per border, I think is better to define the combinations we need and put an specific name:

all 4 borders

only bottom border

only right / left border

the specification you provided is also 4, but the one in the code provides a bit more flexibility.
lets discuss it more plz. @moalobaidat thoughts?

What do you mean?
I provided 3 options only:

one with all borders active, like a button

one for "stacked" elements (border-bottom by default but if we stack from bottom to top the border will be border-top)

one for inline elements, it will be right border by default and left border in RTL mode for this I haven't found any example so I think we can ommit

@abu-seini-maf - Just added my comment above

abu-seini-maf · 2024-11-18T07:21:45Z

src/themes/dark.css

+  --mafui-navigationItem-font-weight-primary: var(--sl-font-weight-normal);
+  --mafui-navigationItem-line-height-primary: var(--sl-line-height-normal);
+  --mafui-navigationItem-letter-spacing-primary: var(--sl-letter-spacing-normal);
+  --mafui-navigationItem-font-color-primary: var(--sl-color-primary-100);


@carlos-verdes @moalobaidat will you also validate the behavior of CSS variables hierarchy:
Component Variables -> Semantic Variables -> Primitives.

For colors what I see is good enough:

--sl-color-secondary-50: var(--sl-color-green-50);

--sl-color-green-50 is the primitive and I think is self explanatory (no comments here)
--sl-color-secondary-50 this is the semantic, based on the policies I think this also fit as they mention 3 colors per site

To be discuss about fonts, in That concept store I think we have just 2, primary and secondary but I'm not sure how many variations we may have on top of that.
I'm not sure about pointing to --sl-font-mono, I think better to point to --sl-font-maint for example

For space semantics I like xs, s, m, etc.

carlos-verdes · 2024-11-18T09:32:57Z

src/themes/dark.css

@@ -258,6 +258,32 @@
  --sl-color-primary-900: var(--sl-color-sky-900);
  --sl-color-primary-950: var(--sl-color-sky-950);

+  /* Secondary */


I think you didn't apply changes to light.css

carlos-verdes · 2024-11-18T09:48:44Z

src/themes/dark.css

+  --mafui-navigationItem-line-height-primary: var(--sl-line-height-normal);
+  --mafui-navigationItem-letter-spacing-primary: var(--sl-letter-spacing-normal);
+  --mafui-navigationItem-font-color-primary: var(--sl-color-primary-100);
+  --mafui-navigationItem-color-background-primary: var(--sl-color-neutral-50);


this is pointing directly to primitive from component, I think we should point to something like --sl-color-background-primary

carlos-verdes · 2024-11-18T09:49:08Z

src/themes/dark.css

+  --mafui-navigationItem-border-width-bottom-primary: 1px;
+  --mafui-navigationItem-border-style-bottom-primary: solid;
+  --mafui-navigationItem-color-border-bottom-primary: var(--sl-color-primary-500);


Same as before

moalobaidat · 2024-11-18T10:07:32Z

src/components/mf-nav-item/mf-nav-item.styles.ts

-    line-height: var(--sl-line-height-normal);
-    letter-spacing: var(--sl-letter-spacing-normal);
-    color: var(--sl-color-neutral-700);
+    font-family: var(--mafui-navigationItem-font-family-primary);


It's a good practice to include a fallback value, especially for properties like color or font-family. Check below example:

font-family: var(--mafui-navigationItem-font-family-primary, Arial, sans-serif);

moalobaidat · 2024-11-18T10:08:49Z

src/components/mf-nav-item/mf-nav-item.styles.ts

+    letter-spacing: var(--mafui-navigationItem-letter-spacing-primary);
+    color: var(--mafui-navigationItem-font-color-primary);
+    background-color: var(--mafui-navigationItem-color-background-primary);
+    border-top: var(--mafui-navigationItem-border-width-top-primary)


If the borders on all sides are meant to be the same, you can simplify this to use a shorthand border property instead of defining each side separately. Like below:

border: var(--mafui-navigationItem-border-width-primary)
var(--mafui-navigationItem-border-style-primary)
var(--mafui-navigationItem-color-border-primary);

moalobaidat · 2024-11-18T10:13:18Z

src/themes/dark.css

+  --mafui-navigationItem-border-style-bottom-primary: solid;
+  --mafui-navigationItem-color-border-bottom-primary: var(--sl-color-primary-500);
+  --mafui-navigationItem-border-width-left-primary:;
+  --mafui-navigationItem-border-style-left-primary:;


Empty Variable Declarations. These variables should either be assigned meaningful values or removed if they are not yet needed.

addition of nav-item component styles and style behavior

137d9f9

abu-seini-maf requested review from carlos-verdes and moalobaidat November 17, 2024 12:14

carlos-verdes reviewed Nov 18, 2024

View reviewed changes

abu-seini-maf commented Nov 18, 2024

View reviewed changes

carlos-verdes reviewed Nov 18, 2024

View reviewed changes

moalobaidat reviewed Nov 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

addition of nav-item component styles and style behavior #28

addition of nav-item component styles and style behavior #28

abu-seini-maf commented Nov 17, 2024

carlos-verdes Nov 18, 2024

abu-seini-maf Nov 18, 2024

carlos-verdes Nov 18, 2024

carlos-verdes Nov 18, 2024

abu-seini-maf Nov 18, 2024

carlos-verdes Nov 18, 2024

carlos-verdes left a comment

carlos-verdes Nov 18, 2024

abu-seini-maf Nov 18, 2024

carlos-verdes Nov 18, 2024

moalobaidat Nov 18, 2024

carlos-verdes Nov 18, 2024

abu-seini-maf Nov 18, 2024

carlos-verdes Nov 18, 2024

abu-seini-maf Nov 18, 2024

carlos-verdes Nov 18, 2024

moalobaidat Nov 18, 2024

abu-seini-maf Nov 18, 2024

carlos-verdes Nov 18, 2024

carlos-verdes Nov 18, 2024

carlos-verdes Nov 18, 2024

carlos-verdes Nov 18, 2024

moalobaidat Nov 18, 2024

moalobaidat Nov 18, 2024

moalobaidat Nov 18, 2024

addition of nav-item component styles and style behavior #28

Are you sure you want to change the base?

addition of nav-item component styles and style behavior #28

Conversation

abu-seini-maf commented Nov 17, 2024

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

carlos-verdes 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment