Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WD-11691] chore: minor UI change for vertical navigation resizing. #797

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

Kxiru
Copy link
Contributor

@Kxiru Kxiru commented Jun 14, 2024

Done

QA

  1. Run the LXD-UI:
    • On the demo server via the link posted by @webteam-app below. This is only available for PRs created by collaborators of the repo. Ask @mas-who or @edlerd for access.
    • With a local copy of this branch, run as described here.
  2. Perform the following QA steps:
    • N/A

Screenshots

image

image

@webteam-app
Copy link

Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't introduce a new concept for scrolling. We currently have this function to set the lower part of the nav to scroll. It works by measuring its height and comparing it to the total available height. This way we get a smooth transition on resize of the browser. It should also work by enabling scrolling on collapse/un-collapse of the accordions in the navigation (like for storage) in all possible height/width combinations.

So either remove that function and find a pure css solution or remove the css solution and rely on the existing one.

src/sass/_pattern_navigation.scss Outdated Show resolved Hide resolved
@Kxiru Kxiru force-pushed the lxd-navigation-vertical-resizing branch from ea04681 to 267317f Compare June 17, 2024 18:46
@Kxiru Kxiru requested a review from edlerd June 17, 2024 18:46
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See screen below:

  1. We shouldn't hide the scrollbar in the navigation (see scrollbar is present on the right)
  2. The top part of the nav is cut, even though there is enough space below. The height of the top part should be fully responsive, so it works in all possible screen widths/heights perfectly.

image

@Kxiru Kxiru force-pushed the lxd-navigation-vertical-resizing branch from 267317f to e46492b Compare June 19, 2024 13:43
@Kxiru Kxiru requested a review from edlerd June 19, 2024 13:43
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the demo server, the navigation doesn't scroll any more. I.e. when I shrink the browser windows height the lower part of the nav stays visible as required, but the middle part is cur with no way of reaching the hidden nav items. Is the demo not updated, or is it a problem with the changes?

@Kxiru
Copy link
Contributor Author

Kxiru commented Jun 19, 2024

On the demo server, the navigation doesn't scroll any more. I.e. when I shrink the browser windows height the lower part of the nav stays visible as required, but the middle part is cur with no way of reaching the hidden nav items. Is the demo not updated, or is it a problem with the changes?

That is interesting... On my local computer and when I run the demo server, I am getting the correct behaviour. @mas-who could you please confirm what you see?

@edlerd , this is the main thing that I had issue with fixing earlier, but I have corrected this and done extensive testing since then. (As well as cleared my cache lol)

@Kxiru Kxiru marked this pull request as ready for review June 19, 2024 15:21
@Kxiru Kxiru force-pushed the lxd-navigation-vertical-resizing branch from e46492b to c4a3be5 Compare June 19, 2024 15:26
@Kxiru
Copy link
Contributor Author

Kxiru commented Jun 19, 2024

That being said, I have found that the top container is not scrollable. I added a fixing line for this in my last commit.

Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA looks good now, the linter has some errors, see below.

Signed-off-by: Nkeiruka <nkeiruka.whenu@canonical.com>
@Kxiru Kxiru force-pushed the lxd-navigation-vertical-resizing branch from c4a3be5 to 84a568d Compare June 19, 2024 16:22
@Kxiru Kxiru requested a review from edlerd June 19, 2024 16:23
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, nice work 👍

Copy link

@piperdeck piperdeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mas-who mas-who merged commit 9f9f47d into canonical:main Jun 25, 2024
12 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants