-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: Pretty Preview OG Tag Data with Social Cards #41
Conversation
Ok, I have reached a good stopping point.
Some refinements to be sure:
|
|
||
const [getOGTagPreview, ogTagPreviewQuery] = | ||
useLazyQuery(OG_TAG_PREVIEW_QUERY) | ||
useQuery(PROJECT_CONFIG_QUERY, { |
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.
How come you're not using a Cell for this?
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.
Because I just wanted to set a flag to show the disabled message section or render the page as per usual. There is no concept or empty state of need for a spinner loading state. I'd then have to wrap everything in a cell and that didn't seem worth it.
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.
So how do you handle loading state now?
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.
To me this looked like a typical example of the "CellPage" people have been asking about for a few years now from time to time. I.e. the page would be empty pretty much, just render a Cell that has all the actual page content inside
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 don't have a loading state - the success and error are done in the OnCompleted and onError handers. Apps don't need to use Cells - we know of very large production apps that do not use that pattern. I would use query and not a cell if I was fetching the nav emu (and did in a preview implementation) as I didn't want to show a loading indication when making menus.
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.
Also, unless I used the onCompleted and onError handlers, when I set/modifed the variable like showDisplayScrion etc and that was used in a useEffect, React would complain about multiple renders because the data and loading return values from the useQuery somehow caused multiple renders of the page. This was the cleanest implementation to simply "get the user config" and present the page according to its info.
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 would use query and not a cell if I was fetching the nav emu (and did in a preview implementation) as I didn't want to show a loading indication when making menus.
Interesting!
I use a cell for loading the menu on https://acm.se. It actually does take long enough to load it so you can see the loading state, even on pretty fast broadband. In a mobile connection it takes even longer. So to me a nav menu that needs to do a gql request is a perfect use case for a cell 🙂
Not saying one way or the other is right or wrong. As always, "it depends" 😄
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.
Hmm now that I think on it given the refactoring I did to make the previewed component it probably could be a cell where …. Well the main query to fetch the config would still be a success … y’know I’ll re think this a bit. A good reason to make in a cell later is if we make some preview images and that’s an RSC component and then a cell fetch pattern would be good to test. As you said — always options.
|
||
import { RefreshIcon } from 'src/icons/Icons' | ||
|
||
const OG_TAG_PREVIEW_QUERY = gql` |
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/should this also be a Cell?
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 don't think it has to be.
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.
Maybe because of the way you pass in setError
etc it's different 🤔
Thanks! I like the audit idea and having all the options determined by the api/service so the reviewers are merely there to style a few bits of information. And should be easy to add new reviewers. I’ll demo for town hall and in test app pick some better / different images 🙂 |
fixes #13
This PR implements a general preview of what a card "should" look like when rendered and unfurled on various site like: Twitter, Facebook, Slack, Discord, LinkedIn.
Note: Draft. Currently just a Twitter card is simulated. Also, dummy data is used, buy the props use the result data:
Basic layout:
The goal is to replicate this preview from https://www.opengraph.xyz/url/https%3A%2F%2Fredwoodjs.com