-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fixes 6223 graph button indistinguishable (#6223). Replaces Button with FloatingButton #6234
Fixes 6223 graph button indistinguishable (#6223). Replaces Button with FloatingButton #6234
Conversation
Log
|
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.
PR Summary
- Replaced
Button
withFloatingButton
in/packages/twenty-front/src/modules/settings/data-model/graph-overview/components/SettingsDataModelOverview.tsx
- Replaced
Button
withFloatingButton
in/packages/twenty-front/src/modules/settings/data-model/objects/SettingsObjectCoverImage.tsx
- Added support for 'to' prop in
/packages/twenty-front/src/modules/ui/input/button/components/FloatingButton.tsx
for navigation functionality
3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
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 @Faisal-imtiyaz123 for the contribution! Looks good, just a small change :)
packages/twenty-front/src/modules/ui/input/button/components/FloatingButton.tsx
Outdated
Show resolved
Hide resolved
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.
PR Summary
(updates since last review)
- Modified
text-decoration
property to always be 'none' in/packages/twenty-front/src/modules/ui/input/button/components/FloatingButton.tsx
- Introduced
to
prop andLink
component fromreact-router-dom
in/packages/twenty-front/src/modules/ui/input/button/components/FloatingButton.tsx
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
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.
Left a last comment!
packages/twenty-front/src/modules/ui/input/button/components/FloatingButton.tsx
Show resolved
Hide resolved
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.
PR Summary
(updates since last review)
- Added
to
prop toFloatingButton
component in/packages/twenty-front/src/modules/ui/input/button/components/FloatingButton.tsx
- Integrated
Link
component fromreact-router-dom
for navigation functionality - Ensured
text-decoration
is always 'none' for the button
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
@@ -87,6 +93,7 @@ const StyledButton = styled.button< | |||
&:focus { | |||
outline: none; | |||
} | |||
text-decoration: none; |
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.
Style: Ensure text-decoration
is set to none
to remove underline from link.
My bad, we actually need the 'to' to be there. I missed the 'as' props that use Link in this case |
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.
PR Summary
(updates since last review)
- No major changes found since last review.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
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.
LGTM, thank you!
fixes #6223