-
Notifications
You must be signed in to change notification settings - Fork 33
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
Fixes inability to delete single profiles + minor UI change - [WD-10834] #769
Fixes inability to delete single profiles + minor UI change - [WD-10834] #769
Conversation
4a5e12e
to
a38bd96
Compare
a28aeab
to
b1a4be8
Compare
Thanks for the changes :) Just one small question regarding some scss update. Just leaving the list of comments addressed based on our in person discussion for future reference:
|
b1a4be8
to
25293de
Compare
LGTM 👍 @piperdeck please have a design pass on this |
This is looking great! A couple things: When a new profile is added, the dropdown defaults to the last profile in the list. This is especially strange behaviour given that the default profile seems to be correctly stickied to the top of the list, so a new profile will never default to the default profile until it's the last profile available. Can we make new attached profiles default to the first in the list rather than the last in the list? CleanShot.2024-05-16.at.15.29.49.mp4When the list of profiles overflows the viewport, I need to repeatedly scroll down in order to keep the "Add Profile" button in view. Could we make it so that it scrolls down automatically to keep the bottom of the list visible when you add a new profile? CleanShot.2024-05-16.at.15.31.32.mp4 |
Thanks, @piperdeck for signing off on the existing work. @mas-who @edlerd , for the amendments, should these become a new PR, a new commit, or simply another amendment to the initial commit? Please advise and I will get started. |
These changes are related to this update, so I'd keep it in the same PR. |
82d8fd5
to
49e2504
Compare
Hi there, @piperdeck , I have implemented your suggestions. Do you mind reviewing the PR so I can grab that sweet Design review +1? |
README.md
Outdated
@@ -5,15 +5,19 @@ Targets small and large scale private clouds. | |||
|
|||
# Install | |||
|
|||
Get the LXD snap | |||
1. Get the LXD snap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this file changed? I think we should undo the changes here to keep the commit clean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might need some assistance here :')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Easiest way is probably just copy the same file content on the master branch on github and paste it into your local file.
@@ -38,12 +36,22 @@ const ScrollableContainer: FC<Props> = ({ | |||
childContainer.setAttribute("style", style); | |||
}; | |||
|
|||
useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's revert all the changes in ScrollableContainer
as the component is used in many other places. I would put this logic in ProfileSelector
and the useEffect
can track the selected
prop.
As a side note, an useEffect
hook with no dependencies array is an anti-pattern in React and should be avoided.
49e2504
to
324f28e
Compare
</div> | ||
))} | ||
{!readOnly && ( | ||
<Button | ||
id={"addProfileButton"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id={"addProfileButton"} | |
id="addProfileButton" |
disabled={unselected.length === 0} | ||
className={"profile-add-btn"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
className={"profile-add-btn"} | |
className="profile-add-btn" |
@@ -53,7 +53,7 @@ const ProfileSelector: FC<Props> = ({ | |||
); | |||
|
|||
const addProfile = () => { | |||
const nextProfile = unselected.pop()?.name; | |||
const nextProfile = unselected[0]?.name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, quick and easy solution 🙂
324f28e
to
94aa2c6
Compare
eeeb06a
to
dbdd1a5
Compare
- Minor UI and scss changes - Profile dropdown defaults to the first profile in the list - Scrollbar 'follows' new additions of profiles. Signed-off-by: Nkeiruka <nkeiruka.whenu@canonical.com>
dbdd1a5
to
8ce0549
Compare
LGTM |
Done
QA
Screenshots