Niloofar/Added menu item component #189
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
niloofar-deriv
requested review from
ali-hosseini-deriv,
yashim-deriv,
azmib-developer,
markw-deriv and
shayan-deriv
as code owners
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
Pull Request Test Coverage Report for Build 9011988756Details
💛 - Coveralls |
shayan-deriv
reviewed
May 9, 2024
shayan-deriv
reviewed
May 9, 2024
| .deriv-menu-item { | ||
| display: flex; | ||
| align-items: center; | ||
| height: 100%; |
There was a problem hiding this comment.
do we need this?
because without any parents this won't be applied on the element.
There was a problem hiding this comment.
Yes @shayan-deriv it's better to be there cause we are using menuItem in the div and it definitely have a parent so we need this.
aizad-deriv
reviewed
May 9, 2024
hasan-deriv
suggested changes
May 9, 2024
Comment on lines
19
to
25
| type TMenuItem = ComponentProps<"a" | "button"> & { | ||
| as: "a" | "button"; | ||
| leftComponent?: ReactNode; | ||
| rightComponent?: ReactNode; | ||
| disableHover?: boolean; | ||
| active?: boolean; | ||
| }; |
There was a problem hiding this comment.
Suggested change
| type TMenuItem = ComponentProps<"a" | "button"> & { | |
| as: "a" | "button"; | |
| leftComponent?: ReactNode; | |
| rightComponent?: ReactNode; | |
| disableHover?: boolean; | |
| active?: boolean; | |
| }; | |
| // Base Props for MenuItem without the 'as' property | |
| interface BaseMenuItemProps { | |
| leftComponent?: ReactNode; | |
| rightComponent?: ReactNode; | |
| disableHover?: boolean; | |
| active?: boolean; | |
| } | |
| // Conditional props that extract the correct attributes for 'a' or 'button' | |
| type ConditionalProps<T extends "a" | "button"> = ComponentProps<T> & BaseMenuItemProps & { | |
| as: T; | |
| }; | |
| // TMenuItem type that uses conditional types based on the 'as' property | |
| type TMenuItem = ConditionalProps<"a"> | ConditionalProps<"button">; |
There was a problem hiding this comment.
I was able to send href attribute when as property was set to button. This type will prevent
There was a problem hiding this comment.
Thanks @hasan-deriv that's a great suggestion but as we talked we have an issue with ref type and since we are going to deliver this component ASAP I'm going to ignore this for now later we can improve the types 🙏
Deploying deriv-ui with
|
| Latest commit: |
7e45efe
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7398830d.deriv-ui.pages.dev |
| Branch Preview URL: | https://niloofar-menu-item.deriv-ui.pages.dev |
shayan-deriv
approved these changes
May 9, 2024
|
🎉 This PR is included in version 1.21.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces the
MenuItemcomponent, designed to render dynamically as either a<button>or an<a>tag based on the passed props.This flexibility allows the component to function in various contexts throughout our application, such as navigating to different pages or performing in-page actions.
Screen.Recording.2024-05-09.at.11.34.09.AM.mov