-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[WEB-3292] fix: workspace switcher validation and ui improvements #6570
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
[WEB-3292] fix: workspace switcher validation and ui improvements #6570
Conversation
Caution Review failedThe pull request is closed. WalkthroughThe update introduces the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as SidebarDropdownItem
participant M as MobX Store
U->>C: Interact with sidebar item
C->>M: Observe state changes
M-->>C: Return updated permissions/state
C->>C: Render updated links & styles
C->>U: Display enhanced component
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
web/core/components/workspace/sidebar/dropdown-item.tsx (2)
51-52
: Consider improving image accessibility.While the styling changes look good, consider enhancing the accessibility of the workspace logo:
- Add
aria-label
to the logo container for screen readers- Use a more descriptive alt text for the logo image instead of the generic "workspace_logo"
<span className={`relative flex h-8 w-8 flex-shrink-0 items-center justify-center p-2 text-base uppercase font-medium border-custom-border-200 ${ !workspace?.logo_url && "rounded-md bg-custom-primary-500 text-white" }`} + aria-label={`${workspace.name} workspace logo`} > {workspace?.logo_url && workspace.logo_url !== "" ? ( <img src={getFileURL(workspace.logo_url)} className="absolute left-0 top-0 h-full w-full rounded object-cover" - alt={t("workspace_logo")} + alt={`${workspace.name} workspace logo`} /> ) : ( (workspace?.name?.[0] ?? "...") )} </span>Also applies to: 58-58
89-106
: Consider extracting duplicated styles into a shared class.The hover styles are duplicated between the settings and member invitation links. Consider extracting these styles into a shared class for better maintainability.
+const linkClassName = "flex border border-custom-border-200 rounded-md py-1 px-2 gap-1 bg-custom-sidebar-background-100 hover:shadow-sm hover:text-custom-text-200 text-custom-text-300 hover:border-custom-border-300"; <> <Link href={`/${workspace.slug}/settings`} - className="flex border border-custom-border-200 rounded-md py-1 px-2 gap-1 bg-custom-sidebar-background-100 hover:shadow-sm hover:text-custom-text-200 text-custom-text-300 hover:border-custom-border-300 " + className={linkClassName} > <Settings className="h-4 w-4 my-auto" /> <span className="text-sm font-medium my-auto">{t("settings")}</span> </Link> <Link href={`/${workspace.slug}/settings/members`} - className="flex border border-custom-border-200 rounded-md py-1 px-2 gap-1 bg-custom-sidebar-background-100 hover:shadow-sm hover:text-custom-text-200 text-custom-text-300 hover:border-custom-border-300 " + className={linkClassName} > <UserPlus className="h-4 w-4 my-auto" /> <span className="text-sm font-medium my-auto"> {t("project_settings.members.invite_members.title")} </span> </Link> </>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/core/components/workspace/sidebar/dropdown-item.tsx
(4 hunks)
🔇 Additional comments (1)
web/core/components/workspace/sidebar/dropdown-item.tsx (1)
1-2
: LGTM! Good improvements to state management and code organization.The addition of MobX's observer wrapper will ensure the component re-renders when relevant observable state changes. The import organization with comments also improves code readability.
Also applies to: 6-6, 12-12, 14-14, 23-23
Description
This PR includes following changes:
Type of Change
References
[WEB-3292]
Summary by CodeRabbit
New Features
Style Improvements