Skip to content
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

Merged
merged 4 commits into from
Mar 10, 2020
Merged

Added updates as per issue #10 fixes all of them #34

merged 4 commits into from
Mar 10, 2020

Conversation

sudipt1999
Copy link
Contributor

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.
1
2

@sudipt1999 sudipt1999 changed the title Set the GET operation as default operation to fast quering around endpoints, fixes #10 Added updates as per issue #10 fixes all of them Mar 10, 2020
@sudipt1999
Copy link
Contributor Author

Updates added to the pull requests are

  1. Added a Clear form button to clear the form fields values since we have to manually delete them all curently.
  2. Added localstorage to the app for storing the properties and resourceIDs fields value, I have tested out them properly but will request a final check from the mentors.

Screenshot from 2020-03-10 14-46-39

@gustavodemorais
Copy link
Contributor

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.

Copy link
Contributor

@gustavodemorais gustavodemorais left a 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:

image

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

@sudipt1999
Copy link
Contributor Author

@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 master branch, I will keep this in mind for the future pr.
Thank you

@sudipt1999
Copy link
Contributor Author

Updates done

  1. Fixed the localstorage error
  2. Placed the clear button at right corner of form also checked for responsiveness!
    Screenshot from 2020-03-10 21-54-23

@sudipt1999
Copy link
Contributor Author

@Guttz I have added changes as requested, kindly review the changes.

Copy link
Contributor

@gustavodemorais gustavodemorais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@gustavodemorais gustavodemorais merged commit caf48e9 into HTTP-APIs:master Mar 10, 2020
@sudipt1999 sudipt1999 deleted the ux-enhancements branch April 4, 2020 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants