-
Notifications
You must be signed in to change notification settings - Fork 204
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
1063 fe hardware montioring monitor gauge #1087
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
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.
Good work on this, @jennifer-gan ! I will take a closer look to the implementation soon
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.
Component looks good!
If we can refactor all the hardcoded colors and fonts to use the theme, or add them to the theme of needed I think we're good to go.
- Use different color for above and below 50%
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.
Thanks for refactoring values to the theme!
I'm wondering if we can make them a bit more reusable though rather than have them defined solely for the gauges?
I think we'll probably be using those font sizes / colors for other charts as well.
It might be nice to make the colors values that can be passed in as props as well to make the component a bit more flexible?
The design and the micro interaction looks very good! |
1. Extract out all hardcoded values to consts 2. Add arial labels to text 3. Handle error boundary when progressValue is not within 0 -100
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.
In my previous review I was suggesting passing the colors in as props so the component can be reused with different colors easily, I don't think we achieve that by passing in the theme? Let me know if I've missed something
I am not sure what should be analyzed in the video provided in the comment. This PR creates, but does not implement the component, did I get it right? |
in the video, the component was implemented and rendered at the bottom (75% gauge) |
I have attached a new one , to demo the new gauge component, this time on top of page, hope this is easier to see and understand
|
- Remove used import - use Clamp to check value boundary - Naming changes
@jennifer-gan can you please delete the code rabbit and llama comments that do not apply? If any of them apply, you can make the changes, then delete |
- move Gauge specific props back to component
yes, tried delete or hide some for both type of AI code reviews, but seems many of them does not allow to delete or hide |
resolved in: 94e4d1b |
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.
I've pushed a commit to decouple the light and dark themes as I don't believe it is a good idea to have one dependent on the other.
I also added JSdocs and added some _
to var names for clariry. Please remember to include JSdocs and example usage in future so we know how to use the component 👍
There are some changes that I think could improve the component, but we can refactor at a later date. I think it's time to merge in this component and move onward.
for the gauge to be shown
Screencast.from.2024-11-05.10-12-18.webm