-
Notifications
You must be signed in to change notification settings - Fork 64
fix: blog share pill click target #692
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,15 +15,18 @@ import { | |
| DialogTrigger, | ||
| } from '@the-monkeys/ui/atoms/dialog'; | ||
| import { toast } from '@the-monkeys/ui/hooks/use-toast'; | ||
| import { twMerge } from 'tailwind-merge'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why did we import twmerge here?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the
Without twmerge, both classes may remain in the final class string, and the resulting styling can depend on Tailwind’s generated CSS order. twMerge ensures the custom trigger styles correctly override the defaults. |
||
|
|
||
| interface BlogShareDialogProps { | ||
| blogURL: string; | ||
| size?: number; | ||
| triggerClassName?: string; | ||
| } | ||
|
|
||
| export const BlogShareDialog: FC<BlogShareDialogProps> = ({ | ||
| blogURL, | ||
| size = 18, | ||
| triggerClassName, | ||
| }) => { | ||
| const copyToClipboard = () => { | ||
| if (navigator.clipboard) { | ||
|
|
@@ -49,7 +52,10 @@ export const BlogShareDialog: FC<BlogShareDialogProps> = ({ | |
| <Dialog> | ||
| <DialogTrigger asChild> | ||
| <button | ||
| className='p-1 flex items-center justify-center cursor-pointer opacity-80 hover:opacity-100' | ||
| className={twMerge( | ||
| 'p-1 flex items-center justify-center cursor-pointer opacity-80 hover:opacity-100 transition-colors', | ||
| triggerClassName | ||
| )} | ||
| title='Share Blog' | ||
| > | ||
| <Icon name='RiShare' size={size} /> | ||
|
|
||
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.
Do we really need this many tailwind properties here?
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.
Most of these classes are not required for the click functionality. They are needed only to preserve the existing pill appearance and add clearer hover/focus feedback.
So, the full list is not strictly required, but the padding, background, rounded shape, and hover/focus styles should remain to keep the intended design and UX.
SO what should i do with them lmk.
I feel it looks fine tho.