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

IS-678: UI changes for album tile #1630

Merged
merged 3 commits into from
Nov 2, 2023
Merged

IS-678: UI changes for album tile #1630

merged 3 commits into from
Nov 2, 2023

Conversation

harishv7
Copy link
Contributor

@harishv7 harishv7 commented Oct 31, 2023

Problem

Implement UI changes for album tile. Currently, our "edit" in the dropdown is just equivalent to clicking the tile and "settings" just does rename of the folder.

Closes IS-678

Solution

Took the liberty to update across images + files, resource room and workspace.

UI should look like this. For full design see Figma link on ticket IS-678

image

Breaking Changes

  • Yes - this PR contains breaking changes
    • Details ...
  • No - this PR is backwards compatible with ALL of the following feature flags in this doc

Before & After Screenshots

BEFORE:
Screenshot 2023-10-31 at 11 25 52 AM

AFTER:

Screenshot 2023-10-29 at 9 28 44 PM
Screenshot 2023-10-29 at 9 28 38 PM
Screenshot 2023-10-29 at 9 28 16 PM
Screenshot 2023-10-29 at 9 28 09 PM

Tests

  • Unit tests (using npm run tests)
  • e2e tests (comment on this PR with the text !run e2e)
  • Smoke tests
    • Ensure clicking on dropdown on image album reflects above UI and you can perform the actions on the dropdown
    • Ensure clicking on dropdown on folders reflects above UI and you can perform the actions on the dropdown
    • Ensure clicking on dropdown on resource category reflects above UI and you can perform the actions on the dropdown
    • Ensure clicking on dropdown on workspace folders reflects above UI and you can perform the actions on the dropdown

@harishv7
Copy link
Contributor Author

Note: Haven't update E2E (if needed)

@harishv7 harishv7 temporarily deployed to staging October 31, 2023 03:34 — with GitHub Actions Inactive
@harishv7 harishv7 temporarily deployed to staging October 31, 2023 03:41 — with GitHub Actions Inactive
Copy link
Contributor

@seaerchin seaerchin left a comment

Choose a reason for hiding this comment

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

lgtm, just curious - editing of folder seems to have been removed wholesale? if this is unintended, pls dismiss

Comment on lines -48 to -53
icon={<BiEditAlt />}
as={RouterLink}
to={`/sites/${siteName}/media/${mediaType}/mediaDirectory/${encodedDirectoryPath}`}
>
<Text>Edit folder</Text>
</ContextMenu.Item>
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if this is covered later on but how are we allowing them to edit the folder in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait current understanding is Edit folder just allows user to rename folder, pls correct me if otherwise

Copy link
Contributor

@sehyunidaaa sehyunidaaa Nov 2, 2023

Choose a reason for hiding this comment

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

  1. "Edit ____" for folder / album / category / page: used to be the same as "open"
  2. "Edit ____" for file / image: used to be the same as "rename" (= also called "Settings" on the modal)

Imo #1 is okay to get rid of becos it's the same as just clicking on the tile
#2 is covered by the new "Rename" option

@sehyunidaaa
Copy link
Contributor

@harishv7 Screenshots look good!

@harishv7 harishv7 merged commit b053fed into develop Nov 2, 2023
10 checks passed
@mergify mergify bot deleted the IS-678-album-tile branch November 2, 2023 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants