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

feat(images): Implement design for image table header MAASENG-4359 #5595

Conversation

abuyukyi101198
Copy link
Contributor

@abuyukyi101198 abuyukyi101198 commented Jan 28, 2025

Done

  • Changed the wording on the (previously) "Download images" button
  • Added icons to the "Delete" and "Select upstream images" buttons
  • Converted the two-line title to a single-line header with the table buttons as MainToolbar.Controls
  • Converted the the "Stop image import" conditional button to a conditional link button next to the region/rack import status
  • Fixed the id mismatch bug that prevented deletion from working properly by introducing custom getRowId returning the record id instead of table row index

QA steps

  • Verify "Delete" and "Select upstream images" buttons have appropriate icons
  • Verify while images are being downloaded, the "Stop image import" link button appears
  • Ensure all the buttons behave as they did before (including "Stop image import")

Fixes

Resolves:

MAASENG-4359

Screenshots

Screenshot 2025-01-28 at 16 35 38 Screenshot 2025-01-28 at 16 36 03

Notes

  • QA uses a placeholder for the source in the "Images synced from x" title, however, the existing default value displays "Images synced from sources"
  • Since the region/rack status information and the new "Stop image import" link button are conditionally displayed, they can disrupt the layout by shifting the buttons down temporarily

Sorry, something went wrong.

@webteam-app
Copy link

@dgtlntv
Copy link
Member

dgtlntv commented Jan 29, 2025

Regarding the first point of the notes:

  • In the QA document I proposed that we could use a skeleton loading rectangle as a placeholder for the source name during loading. If that is too much work / outside of the scope of this work then using "sources" as a placeholder word is fine as well in my opinion.

Regarding the second point of the notes:

  • I think this should become less of an issues if/when we move the "automatically sync images" setting to the source editing form. As there will be a little bit more space for the status update.
  • And I think showing notifications and similar status update as part of the normal content flow will at some point always cause a layout shift. Thats why I would love to move to a better notification system in the UI anyways. But that is a project of its own 😅. If we move the the automatically sync images setting to the form I think it shouldnt cause a layout shift on a desktop screen (which is the primary screen size we design for), so in my opinion this should be ok until we move to a better notification system that is outside of the normal content flow and doesnt disrupt layout.

@abuyukyi101198
Copy link
Contributor Author

abuyukyi101198 commented Jan 29, 2025

Moving the "Automatically sync images" setting into the "Change source" form, any Formik form for that matter, is proving to be problematic because the dispatch used to update the setting interferes with the form's internal rendering (Cannot update a component while rendering a different component). It is definitely something that needs attention, but maybe moving the "Change source" to its own settings page requires its own ticket. MAASENG-4367

@ndv99
Copy link
Contributor

ndv99 commented Jan 29, 2025

QA notes:

  • Missing "Stop image import" link button when images are downloading:
    image

I think that's the only thing I spotted that didn't comply with the frames mentioned in the ticket. LGTM otherwise 👍

@abuyukyi101198
Copy link
Contributor Author

Were the buttons displayed when there was no "Stop image import"?

Because if they weren't, this is due to Redux store and web socket having a delay between one another. Picking a "longer" download should both disable the buttons and display the "Stop image import" link button.

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.

Code is mostly there, could you just add a couple of tests for things like disabling/enabling the "Delete" button when images are selected? TY

@ndv99
Copy link
Contributor

ndv99 commented Jan 29, 2025

Were the buttons displayed when there was no "Stop image import"?

Because if they weren't, this is due to Redux store and web socket having a delay between one another. Picking a "longer" download should both disable the buttons and display the "Stop image import" link button.

Ahhh yes, good point. Selected some bigger images and the link button appeared. In that case, QA +1

…elect upstream images, stop image import and delete
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.

LGTM

@abuyukyi101198 abuyukyi101198 merged commit 811ccda into canonical:main Jan 29, 2025
8 checks passed
@abuyukyi101198 abuyukyi101198 deleted the feat-image-table-header-design-MAASENG-4359 branch January 29, 2025 17:06
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.

4 participants