fix: Airgap: search bar UI was not matching with release #36772
Conversation
WalkthroughThe changes made to the Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
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
Documentation and Community
|
github-actions
bot
added
Airgap
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11250173465. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/components/editorComponents/GlobalSearch/HelpBar.tsx (2)
Line range hint
13-34: Excellent work on simplifying our styling, students!You've done a splendid job removing the conditional styling and ensuring a consistent appearance. The added properties for
flex-grow,gap, andmin-widthare particularly commendable. They'll help our component maintain a proper layout.However, let's take this opportunity for a small improvement. Can anyone tell me why we might want to use CSS custom properties (variables) for these new values?
Consider using CSS custom properties for the new values:
const StyledHelpBar = styled.button` /* ... existing styles ... */ - flex-grow: 0; - gap: 8px; - min-width: fit-content; + flex-grow: var(--help-bar-flex-grow, 0); + gap: var(--help-bar-gap, 8px); + min-width: var(--help-bar-min-width, fit-content); /* ... rest of the styles ... */ `;This change would make our styles more maintainable and easier to adjust in the future. Who can tell me why?
Line range hint
58-62: Bravo on simplifying our component, class!You've successfully removed the conditional rendering, which aligns perfectly with our lesson on ensuring consistency across different versions. The
IconandTextelements are now rendered consistently, which is exactly what we wanted to achieve.Now, let's think about accessibility. Can anyone suggest how we might improve the accessibility of this component?
Let's add an
aria-labelto our button to make it more accessible:<StyledHelpBar className="t--global-search-modal-trigger" data-testid="global-search-modal-trigger" disabled={isProtectedMode} onClick={toggleShowModal} + aria-label="Open global search" > <Icon name={"search-line"} size={"md"} /> <Text italic type={TextType.P3}> {modText()} K </Text> </StyledHelpBar>Who can explain why this addition is important for users who rely on screen readers?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/components/editorComponents/GlobalSearch/HelpBar.tsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
app/client/src/components/editorComponents/GlobalSearch/HelpBar.tsx (2)
3-3: Well done, class! You've simplified our import statements.I'm pleased to see you've removed the unnecessary
useSelectorimport. This is a good step towards keeping our code clean and efficient. Remember, in programming, less is often more!
Line range hint
1-85: Class, let's summarize our review!You've done an excellent job addressing the UI discrepancy between the Airgap and release versions. By removing the feature flag dependency and simplifying the component, you've ensured consistency across different versions. This is precisely what we aimed to achieve with this pull request.
Here's a quick recap of our lessons today:
- We simplified our import statements by removing unnecessary dependencies.
- We streamlined our styling by removing conditional logic and ensuring consistent appearance.
- We simplified our component rendering, removing conditional checks and maintaining core functionality.
For homework, consider implementing the suggested improvements:
- Use CSS custom properties for better maintainability of styles.
- Add an
aria-labelto enhance accessibility.Remember, in software development, we always strive for clarity, consistency, and accessibility. Well done, class!
|
Deploy-Preview-URL: https://ce-36772.dp.appsmith.com |
Description
Global search UI was updated as part of side-by-side layout changes. But since the side-by-side is not yet enabled in Airgap, the global search ui was looking different in release and airgap. This PR address this issue by removing fdeature flag depency for global search UI change.
Fixes #36701
Automation
/ok-to-test tags="@tag.Sanity, @tag.airgap"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11250226767
Commit: 79fc34f
Cypress dashboard.
Tags:
@tag.Sanity, @tag.airgapSpec:
Wed, 09 Oct 2024 08:06:43 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
HelpBarcomponent for a more consistent user experience.IconandTextelements are always displayed.