T 13514/lens share link creator landing #25
Conversation
|
This pull request has been linked to 1 task:
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
@hanahem thank you! Great UX and solid work here!!
Compared to the ticket spec we are not supporting just Publication Id and Profile Id. I think it's ok.... I think most people would use anyway with an app link.
I left few comments that needs addressing.
Instead the "nit" comments are up to you. Not strong about it.
src/components/LinkGenerator.tsx
Outdated
| ) : null} | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Should we give some guidance in case we cannot build the link?
Something on these lines... maybe rework the copy with design?
We are sorry we couldn't build a Lens Share Link at this time.
You can however build it manually adopting one of these patterns:
There was a problem hiding this comment.
Added this when the link couldn't be built.
| const customPublicationUrl = SHARE_PUBLICATION_URL + publicationMatch[1]; | ||
| return { type: "publication", url: customPublicationUrl }; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we should take the opportunity we know the App to add the attribution param: ?by=<app-id>. WDYT?
| return ( | ||
| <main> | ||
| <LinkGenerator /> | ||
| </main> |
There was a problem hiding this comment.
nit: I would consider moving content of the LinkGeneratorcomponent into here until we know it needs to be a separate component. The level of indirection seems not needed here at the this moment in time.
src/components/Spinner.tsx
Outdated
| ) | ||
| } | ||
|
|
||
| export default Spinner |
There was a problem hiding this comment.
nit: favour named exports over default exports. Makes it for easier refactoring/renaming.
src/components/Spinner.tsx
Outdated
| size?: string | ||
| } | ||
|
|
||
| const Spinner: FC<Props> = ({ color, size }) => { |
There was a problem hiding this comment.
nit: since this conversation I am personally not using FC<T> generic anymore. Seem it adds little/no-value.
No description provided.