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: Deactivate & Activate icons should be swapped UI improvements #6796

Merged
merged 2 commits into from
Aug 31, 2024

Conversation

harshit078
Copy link
Contributor

@harshit078 harshit078 commented Aug 29, 2024

Fix

This fixes the minor UI issue #6795 in which it addressed the following two problems-

  1. Fixed the Icon switch
  2. Updated the Delete Modal

Screenshots

Screenshot 2024-08-30 at 1 38 31 AM Screenshot 2024-08-30 at 1 39 13 AM Screenshot 2024-08-30 at 1 39 23 AM

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This pull request addresses UI improvements for field activation/deactivation icons and the delete workspace button, enhancing visual consistency and user understanding.

  • Swapped IconArchive with IconArchiveOff for 'Deactivate' action in SettingsObjectFieldActiveActionDropdown
  • Changed IconArchiveOff to IconArchive for 'Activate' action in SettingsObjectFieldInactiveActionDropdown
  • Updated DeleteWorkspace component: replaced StyledConfirmationButton with Button, added IconTrash, and adjusted styling
  • Improved button appearance in DeleteWorkspace to better represent a dangerous action
  • Simplified ConfirmationModal import in DeleteWorkspace component

3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@FelixMalfait
Copy link
Member

I think the icon for archive/unarchive was the right one :)

Good for the button, we can remove the StyledConfirmationButton if it isn't used anywhere anymore. Thanks!

@lucasbordeau
Copy link
Contributor

@Bonapara Should we switch the icons ?

@lucasbordeau lucasbordeau added blocked: design needed This doesn't seem right and removed -PR: awaiting review labels Aug 30, 2024
@FelixMalfait
Copy link
Member

@lucasbordeau no we shouldn't. But we can merge the button change if we revert the change on icons

@FelixMalfait FelixMalfait added -PR: awaiting author and removed blocked: design needed This doesn't seem right labels Aug 30, 2024
Copy link
Member

@charlesBochet charlesBochet 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 @harshit078

@charlesBochet charlesBochet merged commit c5572f1 into twentyhq:main Aug 31, 2024
3 of 8 checks passed
Copy link

Thanks @harshit078 for your contribution!
This marks your 0th PR on the repo. You're top 0% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

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