-
Notifications
You must be signed in to change notification settings - Fork 490
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
feat: add visual feedback on API address change #1671
feat: add visual feedback on API address change #1671
Conversation
Thanks, @jack-michaud! Assigning some folks to have a look. Please note this week is a little hectic but we'll act as speedily as we can! |
Thanks for the prompt triage @jessicaschilling ! |
Hi @jack-michaud -- sounds like a plan. Drop a comment when you're ready for a final review. 😊 I didn't dig into the code, but the video and screenshots look excellent! The only nit is that we're using snackbars for error messages (example pasted below), so for consistency's sake, probably best to present the error messages that way. (For emphasis, leaving the red border around the entry box until it's cleared by something else - like entering another address - will be helpful.) (FWIW, that error was invoked by entering |
// Updates error based on API connection state. | ||
useEffect(() => { | ||
if (ipfsInvalidAddress) { | ||
setErrorText('The given IPFS API address is invalid'); |
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.
Don't forget to use translations here
- Use notify/snackbar for errors and success messages. - Use notify error message that have translations
Opt to use notify instead
Thanks for the feedback and review @jessicaschilling and @rafaelramalho19 -- FailSuccessI also used the internationalization keys in to eventually give this a custom translation... the |
That's it for Transifex! For the fail state, could you please give the input box a red highlight border? |
… invalid api address
- Shows fail/success border without being selected - Does not refocus if it fails to connect
it fails to connect. When entering in another address, it will refresh the fail state on the input
@jessicaschilling Absolutely! I pulled another feature from the peer "add connection" dialog behavior. For invalid multiaddrs, the submit button is disabled. Thank you for bearing with me in these iterations. |
@jack-michaud This is awesome - thank you! |
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.
@jack-michaud reviewed functionally and this looks and works great, thank you! ❤️
@rafaelramalho19 please review in spare time, and merge when you feel it's ready. (any accessibility concerns?)
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.
LGTM, after my change request we're good to merge 🎉
…ture/api-address-status
- Use triple equals
Thanks for the review, @rafaelramalho19 - I fixed the equals signs, let me know if there's anything else you'd like to see! |
Thanks, @jack-michaud — merged! We very much appreciate your work! If you have the inclination, please feel free to pick up any other issues labeled |
Saw that there was a help wanted tag on #1635, thought I could lend a hand!
Changes
ApiAddressForm.js
)ApiAddressForm.js
)ApiAddressForm.js
)ipfs-provider.js
)ipfs-provider.js
)SettingsPage.js
)Demo
Tried to submit an invalid IPFS API address
Tried unsuccessfully to connect to an IPFS API
(Video) Filling out an API address
Video (filling-out-address.webm)
TODO
ApiAddressForm.css
file in favor of tachyon utility classes (I don't do anything special in the CSS)Closes #1635