-
-
Notifications
You must be signed in to change notification settings - Fork 929
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
docs(api): add refresh button to examples #3301
base: next
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for fakerjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This PR is ready for reviewing and feedback (except some minor details). |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #3301 +/- ##
==========================================
- Coverage 99.97% 99.97% -0.01%
==========================================
Files 2811 2811
Lines 217006 217006
Branches 939 938 -1
==========================================
- Hits 216955 216953 -2
- Misses 51 53 +2 |
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.
The same applies to For the quotes it is a relatively easy fix. Do you think either of the two issues should be fixed? |
preferably yes, the visual glitch when the first refresh happens is a little disconcerting |
Also noticed the refresh button animates weirdly (the circle moves around as its not centered). My preference would be for a more informative button like "Load more examples..." as suggested before, but welcome to suggest other wordings/placements |
That's one of the reasons, this PR is a draft.
I personally prefer the icon, but that just might be because I have a hard time visualizing the text button/it's placement. If we go for text we should propably replace "more" with "new" because they dont get appended. I do plan on adding a hover text/title to it though. |
How readable do you think the current implementation is? |
i dont think its very intuitive what would happen if you press it (and bear in mind hover titles dont work on mobile). So i'd prefer something more explicit. |
I understand your perspective, but I respectfully disagree. The reload symbol is a widely recognized icon in UI/UX design, and it's generally understood by users. Our documentation is aimed at developers, who are likely familiar with this symbol. Regarding the mobile issue, our primary focus has been on optimizing the desktop experience. While mobile functionality is important, the majority of our users access the documentation on desktop. Therefore, we prioritize desktop usability. I appreciate your feedback, but I believe the current design is effective for our target audience. |
Well, I meant the actual source code, not the visualization. I was recently told that my code is hard to read/maintain for other developers. |
Should I
|
Pulling in prettier client-side for this sounds like overkill. I think using JSON-like results with double quotes in the example output sounds simpler. |
Ready for review |
I did some explorative testing on the preview and found that the framework.CKFbI6s_.js:13 TypeError: Cannot read properties of undefined (reading 'lastElementChild')
at J (method.DAik2iG1.js:1:2549)
at Proxy.O (method.DAik2iG1.js:1:2981)
at a (method.DAik2iG1.js:1:1350)
at Qt (framework.CKFbI6s_.js:13:38)
at He (framework.CKFbI6s_.js:13:108)
at HTMLButtonElement.n (framework.CKFbI6s_.js:18:7223) Otherwise the experice feels pretty clean. One weird thing might be the examples of |
The issue was caused by the additional property after the function call.
For now, I added support for a single property after the function call. You can test this with the airline module, just add a property to one of the examples there. There are 3 ways to handle these type errors:
Especially for new methods/during their initial review, it might be helpful to be able to show more results with a simple click. |
If we can find a way where it works for non-released methods even deploy previews I think that's a nice addition to help with QA. |
Co-authored-by: Shinigami <chrissi92@hotmail.de>
Co-authored-by: Shinigami <chrissi92@hotmail.de>
Co-authored-by: Shinigami <chrissi92@hotmail.de>
Ready for review. |
I don't know what is happening on my local machine, but the CSS animation does not get triggered 😕 On my iOS iPhone the animation works 👀 That is very confusing... |
Fixes #3287
Adds a refresh button to the examples without requiring changes to our examples.
All
faker.
invocations are recorded and pasted as a comment after the invocation.Preview