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.
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
LF-4374 Badge component #3486
base: integration
Are you sure you want to change the base?
LF-4374 Badge component #3486
Changes from 5 commits
aa9be7b
a4512bc
7ec52dd
314d21b
3940db8
5308185
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We're aiming to avoid inline CSS, so could you please change the
style
prop toclassName
instead? 🙏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.
Its good we are avoiding the inline css, i have seen a lot here. And sure, i will update accordingly.
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.
Could
handleTooltipOpen
+handleTooltipClose
be combined by letting the state setter to use previous/pending value. React reference if usefulsetOpen(prevState => !prevState)
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.
That can be done if we want the button to behave like toggle(one click open tooltip and another closes it). Honestly speaking, i am little unclear about the behaviour of this component and that is why i came to the meeting to show you guys what i build. Got some feedback and changed accordingly. Even Antonella is not sure about the hover/click functionality for both mobile and desktop and told me that she would let me know after discussing with the team.
So, do let me know what should be the behaviour of this component for onHover and click, for both mobile and desktop and i will make changes accordingly
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.
Honestly its mostly fine the way you have it as an only onHover on desktop and onClick on mobile.
But specifically the style of the button on desktop should not change onClick on desktop though as shown in the first video... unless the full click behaviour opens the tooltip. So either we need to disable click styles on desktop, or add the full onClick open behaviour. I would personally approve either of those two options.
I will tag Loic on the Jira ticket to hopefully get you clarification on this though so you feel confident. Sorry about that lack of clarity -- please always feel free to tag Loic with your UI/UX questions on the Jira ticket too!
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.
Could you explain the reason behind this change? Instead of setting the link as HTML within the content string, I think we should keep this as is and set the content up as a React Node including the tag
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.
My intentions when developing the component was to make it reusable with different title and content in any other section. So, do not want to hard code anything in the component itself. I will change it to use
<Trans>
component from 'react-i18next' package(which i found later), is safer way to handle these situations and set the link when passing the content to the component.Also, it would be nice if you can tell the link address, then i will update that as well.
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.
Would it be a bad idea to pass a className with these styles to the Badge component instead?
I've never seen a raw CSS className used here before, so it would be nice if we could keep things consistent!
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.
It is one of the way by which we can access the parent class in the child component. But, i will see these styles can be attained by passing class to the component and also avoiding the inline css.