-
Notifications
You must be signed in to change notification settings - Fork 40
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
Added updates as per issue #10 fixes all of them #34
Conversation
…values of a endpoint
… them use the values even after refreshing the state
Updates added to the pull requests are
|
Hey @sudipt1999, thanks for the contributions! The localstorage caching is also a good idea. I'll add some modifications we have to work on to merge the PR. |
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.
1 - I'm not able to open the demo due to the following error:
HydraConsole Line 116 - if(Object.keys(object).length === 0)
Probably not handling well when my localstorage is still empty/first time I open the demo.
2 - The Clear button is a nice feature, but I think it would make more sense UX-wise if it was inside the form box. Could you move it to the top-right of the form box?
3 - This PR and the previous one are on the master branch. After we merge this one we will sync the develop and master. And I will kindly ask you for future PRs to always open it at the develop branch.
When you make the changes I'll try to run it again and take a look
@Guttz Thanks for revewing the changes, as asked I will fix the error above also I wil move the clear button to form top. Sorry for opening the PRs at |
@Guttz I have added changes as requested, kindly review the changes. |
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.
Looks good
As described by @Guttz in #10, setting the GET as default would be a better option and would make a general way since it will be default for all endpoints. So, this pr fixes that, a proof of screenshot is attached below kindly let me know if I changes required.