sanity-io / sanity-plugin-seo-pane

Run Yoast's SEO review tools using Sanity data, inside a List View Pane.
MIT License
38 stars 8 forks source link

Add async support to options, moving into SeoPaneComponent #4

Closed xiata closed 3 years ago

xiata commented 3 years ago

For simplicity sake, this makes options.url, options.keywords, options.synonyms all support string, function:string, and function:Promise<string> values, however, I tried to respect the configuration as noted in the README, not the PropTypes, for url since a fixed string doesn't seem very useful anyway.

Not 100% sure if this actually works as expected as I am still in the process of integrating this into my own studio, but I figure you can give a big 👎 if you have something else in mind, or if it is what you did have in mind and it actually works, then, you know go for it.

SimeonGriggs commented 3 years ago

Hey this is great work, and very tidy!

My only concern is throwing errors if any of the options don't exist – consider scenarios like a New Document which is not Published and Keywords won't yet exist. That's why I had those <Feedback /> Components before so it failed more gracefully and provides the user with ... feedback.

xiata commented 3 years ago

Oh, sorry! I should change the error rendering that follows for Request Error to print out error.message if present, otherwise the JSON.stringified version of it.

I moved the checks into the react-query async so they could be handled there.

Edit: subclass unnecessary, just seems better to handle Error objects better since JSON.stringify(error) = '"{}"'

xiata commented 3 years ago

Modified error showing a bit, so actual Error objects can be displayed correctly, only when the user actually wants to see it. Also made it more clear with the error messages where the error came from. I did remove the distinction between Request Error: and Error: since it honestly wasn't any clearer for me to discern between the two. Now when the data response is empty it won't show Error: as the only message too.

image image