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

chore: Adjusted detail page titles for better ux consistency [WD-11696] #803

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

Kxiru
Copy link
Contributor

@Kxiru Kxiru commented Jun 20, 2024

Done

  • Created and adjusted scss classes.

QA

  1. Run the LXD-UI:
  2. Perform the following QA steps:
    • Verify seamless visual difference between a parent page title and a child page title.

Screenshots

image

image

@edlerd
Copy link
Collaborator

edlerd commented Jun 21, 2024

Demo: https://lxd-ui-803.demos.haus/

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.

Thanks for the change, this is much better than before.

I found two issues on QA

  1. The rename case is duplicating the "/"
    Screenshot from 2024-06-21 08-59-42

  2. This was broken before, I just realized it now due to focus on the header: The loading behaviour of the instance and profile detail pages render the header briefly without the prefix. This is causing a jump in the UI, maybe we can change the loading behaviour to immediately render with the prefix? Also for invalid instance names we should render the header with prefix, currently the bare name is rendered:

Screenshot from 2024-06-21 08-59-54

Screenshot from 2024-06-21 08-59-58

image

@Kxiru Kxiru force-pushed the improve-detail-page-breadcrumbs branch 3 times, most recently from 5391234 to 55c13d2 Compare June 21, 2024 11:00
@Kxiru Kxiru requested a review from edlerd June 21, 2024 11:08
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.

Thanks for the improvements, I can confirm all previous issues are resolved.

There is something odd with the rename case: The rename form is not correctly positioned

image

@Kxiru Kxiru force-pushed the improve-detail-page-breadcrumbs branch from 5157c39 to 96cd5b3 Compare June 24, 2024 09:49
@Kxiru Kxiru requested review from edlerd and piperdeck June 24, 2024 09:50
@Kxiru Kxiru marked this pull request as ready for review June 24, 2024 13:38
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 and code looks good, thanks for improving this.

There is one glitch, which existed previously, about spacing. I think adding the suggested classes will fix it: When in a screen with low height and going into the rename mode, previously the height of the header would grow, causing an additional scrollbar to appear. With the changes I am to keep the height unchanged.

src/components/RenameHeader.tsx Show resolved Hide resolved
src/components/RenameHeader.tsx Show resolved Hide resolved
src/components/RenameHeader.tsx Show resolved Hide resolved
@Kxiru Kxiru requested a review from edlerd June 24, 2024 16:01
- Addressed "lag" in loading time by only rendering the title when the instance has loaded.
- Added breadcrumb header to invalid instance page.
- Minor logic alterations to provide reasoning as to why invalid instances cannot be renamed.

Signed-off-by: Nkeiruka <[email protected]>
@Kxiru Kxiru force-pushed the improve-detail-page-breadcrumbs branch from 96cd5b3 to e970fd7 Compare June 24, 2024 17:11
@Kxiru
Copy link
Contributor Author

Kxiru commented Jun 24, 2024

@edlerd , I added the window.dispatchEvent(new Event("resize")); function to enableRename() as discussed on Mattermost.

@piperdeck
Copy link

looking good, nice job on this!

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

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.

LGTM, thanks for improving the headers.

@edlerd edlerd merged commit 6cf0ebe 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

3 participants