-
Notifications
You must be signed in to change notification settings - Fork 1
Clean up how we use links #892
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?
Conversation
|
Preview deployment: https://link-cleanup.preview.avy-fx.org |
82800b8 to
26333bc
Compare
busbyk
left a comment
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 some comments and had an idea: #900
Psyched to get this consolidated/consistent.
docs/coding-guide.md
Outdated
| └── config.ts | ||
| ``` | ||
|
|
||
| ## Button vs CMSLink |
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'm not clear on the intent for Button, ButtonLink, and CMSLink from these docs / from usage in general.
The use of Button is clear - it's just a button that's not a link. Although I'm surprised that PostHog autocaptures clicks on buttons that aren't wrapped in links / have a child. Is that true?
The difference in usage of ButtonLink and CMSLink is fuzzy. They both render a Button component with a Link inside it with pretty much the same props.
The only things CMSLink does differently is:
- Accepts an internal reference (a Payload document from pages, posts, or builtInPages) and uses handleReferenceURL to determine the internal url
- Uses a custom PostHog event capture which includes the tenant
I get that ButtonLink is specifically for external URLs but I'm definitely feeling like these could just be a single component.
I wonder if we wouldn't want that custom PostHog event capture with tenant context on external links too.
Using CMSLink (the naming specifically being the issue) is confusing for explicitly external links but I think passing a reference (i.e. Payload document) into a ButtonLink component wouldn't be too confusing. Maybe reference isn't the best prop name but 🤷.
I think it would be clearer to just have a single ButtonLink which should be used for all links (internal and external) that should be styled as buttons. If you want a link that's not styled as a button you can use a pattern similar to ImageLinkGridBlock where you use handleReferenceURL and Link.
I don't think we even really need to call out the usage of Button here necessarily except maybe to say: don't use an onClick handler to programmatically navigate unless there's a use case that makes sense but...that could go without saying idk.
Thoughts?
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.
Great thoughts, I consolidated CMSLink into Button link in c452956
The use of Button is clear - it's just a button that's not a link. Although I'm surprised that PostHog autocaptures clicks on buttons that aren't wrapped in links / have a child. Is that true?
Yes that is true - check out this dashboard
The only things CMSLink does differently is:
- Accepts an internal reference (a Payload document from pages, posts, or builtInPages) and uses handleReferenceURL to determine the internal url
- Uses a custom PostHog event capture which includes the tenant
I get that ButtonLink is specifically for external URLs but I'm definitely feeling like these could just be a single component.
I was trying for CMSLink to handle anything that had an admin interface and ButtonLink was anything we linked to in the code, but I forgot CMSLink can have an external URL. As I mentioned everything has been consolidated in c452956
Using CMSLink (the naming specifically being the issue) is confusing for explicitly external links but I think passing a reference (i.e. Payload document) into a ButtonLink component wouldn't be too confusing. Maybe reference isn't the best prop name but 🤷.
LinkFeature uses doc. Take a look and let me know if we should rename the prop.
I don't think we even really need to call out the usage of Button here
Agreed and updated in c452956
…onfig to filter out other tenant documents
Migration Safety CheckFound 8 potential issues: 20260128_213937_rename_appearance_to_variant.ts Warning (line 6): ALTER keyword detected - review for data loss sql`ALTER TABLE \`home_pages_blocks_link_preview_cards\` RENAME COLUMN "button_appearance" TO "button_variant";`,Warning (line 6): RENAME keyword detected - review for data loss sql`ALTER TABLE \`home_pages_blocks_link_preview_cards\` RENAME COLUMN "button_appearance" TO "button_variant";`,Warning (line 9): ALTER keyword detected - review for data loss sql`ALTER TABLE \`_home_pages_v_blocks_link_preview_cards\` RENAME COLUMN "button_appearance" TO "button_variant";`,Warning (line 9): RENAME keyword detected - review for data loss sql`ALTER TABLE \`_home_pages_v_blocks_link_preview_cards\` RENAME COLUMN "button_appearance" TO "button_variant";`,Warning (line 12): ALTER keyword detected - review for data loss sql`ALTER TABLE \`pages_blocks_link_preview_cards\` RENAME COLUMN "button_appearance" TO "button_variant";`,Warning (line 12): RENAME keyword detected - review for data loss sql`ALTER TABLE \`pages_blocks_link_preview_cards\` RENAME COLUMN "button_appearance" TO "button_variant";`,Warning (line 15): ALTER keyword detected - review for data loss sql`ALTER TABLE \`_pages_v_blocks_link_preview_cards\` RENAME COLUMN "button_appearance" TO "button_variant";`,Warning (line 15): RENAME keyword detected - review for data loss sql`ALTER TABLE \`_pages_v_blocks_link_preview_cards\` RENAME COLUMN "button_appearance" TO "button_variant";`,Review these patterns and add backup/restore logic if needed. See |
* use type guard instead of type assertions * updating type assertion record
3c38947 to
8d46264
Compare
Description
This PR cleans up for we use
ButtonandCMSLinkcomponents. We currently use buttons in 3 different ways:From the linked issue:
I assume the second part of this is referring to Posthog events. All link clicks are autocaptured by posthog.
Related Issues
Resolves #685
Resolves #753
Key Changes
ButtonLinkcomponent and consolidatesCMSLinktoButtonLinksince we were repeating the components patterndestructiveandlinkEventTableandEventPreviewto use properButtoncomponent instead of incorrectly usedCMSLinkclearIrrelevantLinkValuestolinkFieldlinkToPageOrPosttolinkFieldlinkToPageOrPostforquickLinksconfig since the type isarrayinstead ofgroupdefaultLexicalurl validationHow to test
There should be no visual differences
Migration
Rename
button_appearancetobutton_variantFuture enhancements / Questions
We could use
linkFieldinnavLink, since the configs are identical. UsinglinkFieldwill changenewTabto be the second column which will cause a migration that will create a new table and drop the old one which seems invasive for the change. Do we want to do this?