-
Notifications
You must be signed in to change notification settings - Fork 31
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
Popup enrichment from wikidata #31
base: master
Are you sure you want to change the base?
Conversation
@jessisena let me know what you think |
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.
@jsanz this is amazing! 👏 thanks! I've let some little comments in the code, but as you said I think is a great starter point.
I have only 2 concerns about the popup:
- the "flashing effect" while loading plus the popup changing size, and
- the best way to visualize the new data in small devices/screens
For the 1st one, I'm thinking that maybe is worth it to fill the popup from the beginning with some kind of placeholders where the final data should appear. And, in case eventually the data is not there, it's also a way to show this fact (the same way that the Wikipedia button is being shown disabled when there's no wikipedia link available).
Other ideas, opinions and/or suggestions are welcome 🙂
Thanks for the review @jessisena, I understand then this functionality is desirable.
Regarding the UI:
Anything else I should take care of? |
Yes, this feature is absolutely desirable @jsanz 🚀 Thanks for addressing the comments and your willingness to add the SPARQL explanation. And about the UI, I've just reviewed the popup (mobile and desktop) and actually I think we could increase and establish the same size for both without problems. For example, the width could be something between 240px or 200px, WDYT? And about the design, my concern about adding the image as a background for the full popup is readability and visibility in general 🙄 but you can tell me better once you try it 👍 Actually, I was thinking about a full re-style of the popup to something like this horizontal design: or another option is to keep the verticality (common style): Although I personally prefer the 1st one 🙂 |
I'll give the image as a background a try and if I see it readable I'll share it. I like the common style option because we can use it both for mobile and desktop but it shouldn't hard to implement a responsive layout. |
@jessisena please take a look at the changes:
It's not perfect and there are some cases still to handle like a better signaling when no data has been retrieved from wikidata, but I think it's ready for you to check |
@jessisena last commit ab6f56b added:
The queries to Wikidata now are not encoded and easier to read. I still need to document them somewhere. I'll try to do this ASAP but the rest of the code is ready for proper review. |
BTW the caches are now for a single day, we may want to extend that a bit but totally up to you. At the end of the export const wikidataExpireCache = 1 * 24 * 3600 * 1000; |
Co-authored-by: Jessica Sena <[email protected]>
Co-authored-by: Jessica Sena <[email protected]>
Much better @jessisena, thanks for the suggestions. I've also added details about the queries in the README files. Let me know if there's anything else to improve. About the Wikidata cache, is one day enough, or should we increase it? My reasoning for that figure is that the website is mostly visited by occasional users so saving repeated queries on a single session seems fine to me, but maybe you think it's better to increase the data retention 🤔 |
This is a prototype since I don't have much knowledge about the display but it should be a good starting point if it's accepted.
Closes #30