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.

Sign up for GitHub

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

Jump to bottom

feat: move collapsible forms to sidepanel #5263

Merged
merged 10 commits into from
Jan 16, 2024
Merged

feat: move collapsible forms to sidepanel #5263

merged 10 commits into from
Jan 16, 2024

Conversation

Jay-Topher
Copy link
Contributor

@Jay-Topher Jay-Topher commented Jan 9, 2024 •
edited by jira bot
Loading

Done

  • Moved collapsible forms in MAAS UI to sidepanels
  • Refer to MAASENG-2410 for an itemized list of components

QA steps

  • Visit MAAS UI and ensure that all formerly collapsible forms have been moved to sidepanel

Fixes

Fixes: MAASENG-2410

Screenshots

Before

image

After

image

Notes

  • While QAing, please feel free to list out any other components you notice that may not have been captured in the original ticket

@webteam-app
Copy link

Demo starting at https://maas-ui-5263.demos.haus

@Jay-Topher Jay-Topher force-pushed the MAASENG-2410-move-collapsible-forms-to-sidepanel branch from 4e677f4 to 6d8ff61 Compare January 9, 2024 09:27
ndv99
ndv99 previously approved these changes Jan 9, 2024
View reviewed changes
Copy link
Contributor

@ndv99 ndv99 left a comment

Choose a reason for hiding this comment

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

Phew, that took a while! But I'm happy to say, LGTM

petermakowski
petermakowski suggested changes Jan 10, 2024
View reviewed changes
Copy link
Contributor

@petermakowski petermakowski left a comment

Choose a reason for hiding this comment

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

Thanks @Jay-Topher - this is a big PR! I'm requesting changes until I have QA'd and reviewed in full.

One thing I found so far:

Inner strip is no longer necessary as the content is wrapped in content section. Please remove, as this increases the padding and is incorrect.

Google Chrome screenshot 001290@2x

petermakowski
petermakowski reviewed Jan 10, 2024
View reviewed changes
Copy link
Contributor

@petermakowski petermakowski left a comment

Choose a reason for hiding this comment

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

I found more issues, more info below. Please tackle them one by one and commit and push your changes as you go.

src/app/pools/components/PoolForm/PoolForm.tsx Show resolved Hide resolved
src/app/pools/components/PoolDeleteForm/PoolDeleteForm.tsx Show resolved Hide resolved
petermakowski
petermakowski suggested changes Jan 10, 2024
View reviewed changes
Copy link
Contributor

@petermakowski petermakowski left a comment

Choose a reason for hiding this comment

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

Looks like you missed the Change source form on the images page.

Google Chrome screenshot 001302@2x

Google Chrome screenshot 001304@2x

@petermakowski
Copy link
Contributor

Image delete confirmation form is not displayed in the side panel.

Google Chrome screenshot 001306@2x

@petermakowski
Copy link
Contributor

@Jay-Topher Thanks for the fixes so far. A number of issues I reported is still unresolved (e.g. including image delete form). In addition to this I found:

  • Tag edit form is not displayed in a side panel

Google Chrome screenshot 001314@2x

  • None of the side panels on the pools page close when pressing "esc".

  • Delete tag side panel content has incorrect font size (this isn't directly linked to this PR so feel free to create a new issue linking to this comment if you decide not to do it now).

Google Chrome screenshot 001316@2x

petermakowski
petermakowski suggested changes Jan 10, 2024
View reviewed changes
Copy link
Contributor

@petermakowski petermakowski left a comment

Choose a reason for hiding this comment

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

I recommend to break it down into smaller, more manageable pull requests to make it easier to review and reduce the likelihood of overlooking errors.

src/app/base/components/NetworkActionRow/NetworkActionRow.tsx Outdated Show resolved Hide resolved
...pp/base/components/node/StorageTables/AvailableStorageTable/BulkActions/BulkActions.test.tsx Show resolved Hide resolved
@Jay-Topher Jay-Topher force-pushed the MAASENG-2410-move-collapsible-forms-to-sidepanel branch 2 times, most recently from 243ae83 to 87f6c4a Compare January 15, 2024 10:02
@Jay-Topher
Copy link
Contributor Author

@Jay-Topher Thanks for the fixes so far. A number of issues I reported is still unresolved (e.g. including image delete form). In addition to this I found:

  • Tag edit form is not displayed in a side panel

Google Chrome screenshot 001314@2x

  • None of the side panels on the pools page close when pressing "esc".
  • Delete tag side panel content has incorrect font size (this isn't directly linked to this PR so feel free to create a new issue linking to this comment if you decide not to do it now).

Google Chrome screenshot 001316@2x

I'll work on the font size in a separate PR so as to keep this one on track, thanks

@petermakowski
Copy link
Contributor

petermakowski commented Jan 15, 2024 •
edited
Loading

@Jay-Topher There's one action that still needs to be moved to a side panel but let's do that in another PR, this one is big enough as it is. Please look into it right after this: https://warthogs.atlassian.net/browse/MAASENG-2572

Google Chrome screenshot 001383@2x
Google Chrome screenshot 001385@2x

petermakowski
petermakowski suggested changes Jan 15, 2024
View reviewed changes
Copy link
Contributor

@petermakowski petermakowski 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 much better. Let's just make sure we fix accessibility issue below which is still unresolved.

Side panels within the preferences (e.g. /MAAS/r/account/prefs/ssl-keys) and pools (/MAAS/r/pools) pages do not close after pressing the ESC key.

@Jay-Topher Jay-Topher force-pushed the MAASENG-2410-move-collapsible-forms-to-sidepanel branch 2 times, most recently from ffe9294 to 264da81 Compare January 15, 2024 15:59
@Jay-Topher Jay-Topher force-pushed the MAASENG-2410-move-collapsible-forms-to-sidepanel branch from 264da81 to c7c81de Compare January 15, 2024 16:05
@ndv99 ndv99 dismissed their stale review January 15, 2024 16:21

(outdated)

petermakowski
petermakowski approved these changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@petermakowski petermakowski left a comment

Choose a reason for hiding this comment

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

@Jay-Topher Noticed 2 more forms that we missed and should be moved to a side panel, created tasks, please work on this afterwards: https://warthogs.atlassian.net/browse/MAASENG-2575 https://warthogs.atlassian.net/browse/MAASENG-2576

@petermakowski petermakowski enabled auto-merge (squash) January 16, 2024 08:13
@Jay-Topher
Copy link
Contributor Author

Sure, will do, thanks

@petermakowski petermakowski merged commit d4949f2 into canonical:main Jan 16, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petermakowski petermakowski petermakowski approved these changes

@ndv99 ndv99 ndv99 left review comments

Assignees
No one assigned
Labels
Review: Code +1 Review: QA +1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Jay-Topher @webteam-app @petermakowski @ndv99