Bump to vanilla-4.7.0 #13548

petesfrench · 2024-02-07T15:00:58Z

Done

Bump to vanilla-4.7.0
removes an unused class

QA

Check that nothing is broken after upgrading vanilla

webteam-app · 2024-02-07T15:01:02Z

Demo starting at https://ubuntu-com-13548.demos.haus

codecov · 2024-02-07T15:03:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4429156) 74.41% compared to head (47bdb30) 74.41%.
Report is 92 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #13548   +/-   ##
=======================================
  Coverage   74.41%   74.41%           
=======================================
  Files         107      107           
  Lines        2838     2838           
  Branches      946      946           
=======================================
  Hits         2112     2112           
  Misses        702      702           
  Partials       24       24

mtruj013

Couple meganav bugs:

bubble nav items aren't aligned as expected:

vs:

Search icon appears on new line on landing:

mtruj013 · 2024-02-07T15:29:18Z

static/sass/styles.scss

@@ -1366,20 +1366,6 @@ $color-link-dark: #69c !default;
  }
 }

-// XXX: @bartaz: this will be covered in Vanilla
-// can be removed once https://github.com/canonical/vanilla-framework/issues/4875 is done and released
-.p-logo-section.has-misaligned-images {


I haven't finished qa'ing all the pages that use this, but it looks like the logo sections in https://ubuntu-com-13548.demos.haus/financial-services still look buggy without it compare to prod

Also, this seems to just remove the comment, but not actual code. Is this intended?

bartaz · 2024-02-07T15:39:58Z

Couple meganav bugs:

@petesfrench @mtruj013 The main change in 4.7.0 was the increased padding of top navigation items. With 4.7.0 top nav items have padding equal to grid margin width (on large screens). (1.5rem vs previous 1rem).
So indeed it may cause some issues there. If anything was aligned to nav before, this will need to be realigned.

Similarly if there is a lot of elements in the nav (and meganav seems to be very packed) the $breakpoint-navigation-threshold may need to be adjusted to drop to mobile view as soon as they don't fit.

If there are any issues that make the new padding too large for meganav ping me and @lyubomir-popov, as maybe we would need some exception for such dense navigation…

petesfrench · 2024-02-08T09:05:04Z

@bartaz @lyubomir-popov In the case of the meganav we already use a custom padding-right: 2.5rem to account for the down arrow and the additional 0.5rem on each button will make it too wide. In this situation would it be viable to maintain the padding-left: 1rem that we are currently using?

bartaz · 2024-02-09T08:58:15Z

Also, 4.7.0 introduces p-navigation__row--25-75 that is more reliable way to implement grid in top nav than using row and cols.

https://vanillaframework.io/docs/examples/patterns/navigation/default

bartaz · 2024-02-12T10:45:36Z

static/sass/_pattern_navigation.scss

@@ -200,6 +200,10 @@ $meganav-height: 3rem;
    }

    .p-navigation__row--25-75 {
+      .p-navigation__items:first-child {


Why is this needed? p-navigation__row--25-75 was added specifically to avoid need for custom styling to match the grid.

Ah, ok, it's because you override the left padding on the nav items, so you need to compensate.

The custom padding around the buttons (due to the chevron) means it doesn't align by default

bartaz

Code looks good to me (with couple comments below), but I don't know which parts are relevant for QA to make sure there are no regressions.

templates/templates/_navigation.html

bartaz · 2024-02-13T14:08:22Z

static/sass/_pattern_navigation.scss

      margin-right: 0;
      max-height: calc(100vh - 3rem);
      width: auto;

      .p-navigation__items {
        width: 100%;

+        & > .p-navigation__item--dropdown-toggle > .p-navigation__link {


It's probably out of scope for this PR, as it doesn't originate here, but the way the navigation is implemented here, with A LOT of overrides of very specific Vanilla class names is quite fragile and possibly problematic whenever we update something in Vanilla.

Variants like these ideally should be implemented with different class names from Vanilla. So instead of overriding & > .p-navigation__item--dropdown-toggle > .p-navigation__link, you would define p-navigation__link--dense (or whatever is the purpose of this) and use this new class name instead.

But as I mentioned, it's not just this one thing in this PR, the whole navigation seems to be written this way, so I mention this as maybe a potential for future improvements/refactoring.

Ok, I have made a separate issue to tackle this as not to block this work https://warthogs.atlassian.net/browse/WD-8935

juanruitina · 2024-02-15T15:43:02Z

I see the search button have some padding that didn't before. This might seem minor, but it is reducing the gap between "Get Ubuntu" and "All Canonical", making it barely noticeable particularly when the user is logged in.

There are some alignment issues on the mobile navigation, at the left of the nav items:

petesfrench · 2024-02-16T10:25:00Z

@lyubomir-popov could you give this a look please

juanruitina · 2024-02-16T10:30:02Z

I see the search button have some padding that didn't before. This might seem minor, but it is reducing the gap between "Get Ubuntu" and "All Canonical", making it barely noticeable particularly when the user is logged in.

Elaborating a bit on this, see these screenshots:

This is what it looks like in the live version:

This is what it looks like in the demo:

See how the gap between "Get Ubuntu" and "All Canonical" virtually disappears, making it all look like one long list instead of two, one left aligned the other right aligned. This arguably increases cognitive download and hinders the sense of hierarchy that separating them provides. @lyubomir-popov thoughts on it?

bartaz · 2024-02-16T11:32:00Z

@juanruitina @petesfrench There seems to be some additional padding in this PR on the right.

What is live seems correct: search button (when hovered) should end exactly where grid ends:

On the demo there is some additional "padding" that squeezes the navigation:

But I can't figure out what causes this. It's not actuall padding or margin, it seems like the items in the nav are not spanning whole width of available space.

bartaz · 2024-02-16T11:37:00Z

@petesfrench I think I see what the issue is.

You have all the nav items in a single p-navigation__items list.

But if you want to have 2 sets: one left aligned, and the other right aligned, you need two separate p-navigation__items lists, not one.

Compare to Vanilla home page:

https://vanillaframework.io/

The misalignment is caused because the first navigation items list is shifted to the left (to align with the grid). So if it includes the right aligned items, those are shifted as well. Right aligned items need to be in separate list.

petesfrench · 2024-02-20T09:25:23Z

@bartaz @juanruitina I have added a negative margin to the top nav so it aligns. The reason for having them all in my list is to simplify the JS logic that handles the navigation. Sticking to the vanilla pattern would be better but would require some refactoring of the code which I would like to avoid right now. What do you think?

petesfrench · 2024-02-21T13:49:13Z

Merging without 4.8.0 as it requires more SCSS changes and would grow the scope of this PR.

Bump to vanilla-4.7.0

a93e907

petesfrench added Review: QA needed Review: Code needed labels Feb 7, 2024

mtruj013 reviewed Feb 7, 2024

View reviewed changes

Revert navigation padding

1ff5d68

petesfrench force-pushed the vanilla-4.7.0 branch from 2f588ef to 1ff5d68 Compare February 8, 2024 09:01

Update global-nav padding to align with content

8129e0b

petesfrench added 2 commits February 9, 2024 15:13

Add p-navigation__row--25-75

d372cd8

Make css adjustments to fit new vainlla pattern

a258404

bartaz reviewed Feb 12, 2024

View reviewed changes

petesfrench requested a review from bartaz February 13, 2024 13:43

bartaz approved these changes Feb 13, 2024

View reviewed changes

bartaz added Review: Code +1 (with changes) and removed Review: Code needed labels Feb 13, 2024

Apply changes from cpde review

7c88ede

Re-align secondary nav

53dfc5d

petesfrench added Review: Design needed and removed Review: QA needed labels Feb 16, 2024

Make top nav align right

b9dbac5

Decrease breakpoint-threshold

7e45591

lyubomir-popov added Review: Design +1 and removed Review: Design needed labels Feb 20, 2024

Format SCSS

47bdb30

petesfrench merged commit 478db36 into main Feb 21, 2024
26 of 28 checks passed

petesfrench deleted the vanilla-4.7.0 branch February 21, 2024 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump to vanilla-4.7.0 #13548

Bump to vanilla-4.7.0 #13548

petesfrench commented Feb 7, 2024 •

edited

Loading

webteam-app commented Feb 7, 2024

codecov bot commented Feb 7, 2024 •

edited

Loading

mtruj013 left a comment

mtruj013 Feb 7, 2024 •

edited

Loading

bartaz Feb 13, 2024

bartaz commented Feb 7, 2024 •

edited

Loading

petesfrench commented Feb 8, 2024

bartaz commented Feb 9, 2024

bartaz Feb 12, 2024

bartaz Feb 12, 2024

petesfrench Feb 13, 2024

bartaz left a comment •

edited

Loading

bartaz Feb 13, 2024

petesfrench Feb 15, 2024

juanruitina commented Feb 15, 2024

petesfrench commented Feb 16, 2024

juanruitina commented Feb 16, 2024

bartaz commented Feb 16, 2024 •

edited

Loading

bartaz commented Feb 16, 2024

petesfrench commented Feb 20, 2024

petesfrench commented Feb 21, 2024

Bump to vanilla-4.7.0 #13548

Bump to vanilla-4.7.0 #13548

Conversation

petesfrench commented Feb 7, 2024 • edited Loading

Done

QA

webteam-app commented Feb 7, 2024

codecov bot commented Feb 7, 2024 • edited Loading

Codecov Report

mtruj013 left a comment

Choose a reason for hiding this comment

mtruj013 Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

bartaz Feb 13, 2024

Choose a reason for hiding this comment

bartaz commented Feb 7, 2024 • edited Loading

petesfrench commented Feb 8, 2024

bartaz commented Feb 9, 2024

bartaz Feb 12, 2024

Choose a reason for hiding this comment

bartaz Feb 12, 2024

Choose a reason for hiding this comment

petesfrench Feb 13, 2024

Choose a reason for hiding this comment

bartaz left a comment • edited Loading

Choose a reason for hiding this comment

bartaz Feb 13, 2024

Choose a reason for hiding this comment

petesfrench Feb 15, 2024

Choose a reason for hiding this comment

juanruitina commented Feb 15, 2024

petesfrench commented Feb 16, 2024

juanruitina commented Feb 16, 2024

bartaz commented Feb 16, 2024 • edited Loading

bartaz commented Feb 16, 2024

petesfrench commented Feb 20, 2024

petesfrench commented Feb 21, 2024

petesfrench commented Feb 7, 2024 •

edited

Loading

codecov bot commented Feb 7, 2024 •

edited

Loading

mtruj013 Feb 7, 2024 •

edited

Loading

bartaz commented Feb 7, 2024 •

edited

Loading

bartaz left a comment •

edited

Loading

bartaz commented Feb 16, 2024 •

edited

Loading