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

fix(detailed pages): Make delete btns more visible on detail pages WD-3870 #353

Merged
merged 1 commit into from
May 31, 2023

Conversation

aaryanporwal
Copy link
Member

@aaryanporwal aaryanporwal commented May 24, 2023

Done

  • Previously lxd-ui was only using delete icons on detail pages (instance, profile and storage).
  • The delete icons were hard to see, and were often missed by the users.
  • Replaced with "Default" vanilla-framework buttons on large screens, and only icon on mobile viewport.

Fixes [list issues/bugs if needed]

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 @lorumic or @edlerd for access.
    • With a local copy of this branch, run as described in the Readme.
  2. Perform the following QA steps:
    • [List the steps to QA the new feature(s) or prove that a bug has been resolved]

Screenshots

Before:
image
After:
image
image

@webteam-app
Copy link

Demo starting at https://lxd-ui-353.demos.haus

Copy link
Contributor

@lorumic lorumic left a comment

Choose a reason for hiding this comment

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

Some QA comments:

  • The button in its "expanded" form should probably not be dense, correct me if I'm wrong @al-bakalova @edlerd
  • It should not break the layout for instances with a long name:
    image
  • The "collapse on small screen" behaviour is not implemented:

When resizing, we should turn the big button into a small icon button, or just “Delete” one. Let’s explore this together.

src/pages/instances/actions/DeleteInstanceBtn.tsx Outdated Show resolved Hide resolved
src/pages/profiles/actions/DeleteProfileBtn.tsx Outdated Show resolved Hide resolved
@al-bakalova
Copy link

@aaryanporwal on top of Michele's feedback, please review the "Delete instance" and "Delete profile" buttons. We need less space before and after the label. Also, please remove the delete icon. It should only be visible on smaller screens to replace the label.

@lorumic
Copy link
Contributor

lorumic commented May 25, 2023

@aaryanporwal on top of Michele's feedback, please review the "Delete instance" and "Delete profile" buttons. We need less space before and after the label. Also, please remove the delete icon. It should only be visible on smaller screens to replace the label.

So, to recap: on small screens, we keep what we have now (icon only, no borders etc). On medium screens and bigger, we show a default button with the label only "Delete instance/profile" and no icon. Is this a good summary @al-bakalova ?

@al-bakalova
Copy link

@lorumic Yes! I hope this makes sense. When we are dealing with longer instance or profile names, we can probably use the icon button earlier. I don't know if this is possible from dev point of view, so I'd leave the decision to you and Aaryan!

@lorumic
Copy link
Contributor

lorumic commented May 25, 2023

@lorumic Yes! I hope this makes sense. When we are dealing with longer instance or profile names, we can probably use the icon button earlier. I don't know if this is possible from dev point of view, so I'd leave the decision to you and Aaryan!

It's probably easier to constrain the appearance of the button to the screen size only. So, on medium and bigger screens, the style of the other elements in the header (breadcrumbs and actions) should be updated to accommodate a bigger button than the one with the icon only, and be opportunely responsive.

src/pages/instances/actions/DeleteInstanceBtn.tsx Outdated Show resolved Hide resolved
src/pages/profiles/actions/DeleteProfileBtn.tsx Outdated Show resolved Hide resolved
src/pages/instances/actions/DeleteInstanceBtn.tsx Outdated Show resolved Hide resolved
src/pages/profiles/actions/DeleteProfileBtn.tsx Outdated Show resolved Hide resolved
update breakpoint, btn look and text and new hook
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, thank you for applying the changes.

@al-bakalova
Copy link

@aaryanporwal Good job! Looks good to me! Thank you for applying all these changes!

@edlerd edlerd merged commit 4b69109 into canonical:main May 31, 2023
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.

5 participants