-
-
Notifications
You must be signed in to change notification settings - Fork 123
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
Creating more UX friendly alerts #134
Comments
Banner notifies the user that a class has been created, it waits for a set amount of times, approximately 5 seconds, before the page refreshes. Banner uses the toastify banner like the one used for issue freeCodeCamp#134 alert banner Co-Authored-By: yeeao <[email protected]> Co-Authored-By: fadibuni <[email protected]>
…sses ClassInviteTable, modal, and index with toastify elements have been modified and trimmed.
I will be working on this! From code labs. |
@GuillermoFloresV How can I reproduce this error? |
Broader Context:
• @Komal914 asked me in CodeDay Slack
• I am replying to @Komal914 here in GitHub: Hi @Komal914 and @GuillermoFloresV, I have received the vanilla JS alerts that @GuillermoFloresV alluded to. Here are three examples of vanilla JS alerts in the Admin view:
|
So, right as we change the role in prisma to admin, the errors pop up? @lloydchang |
Hi there @Komal914 , sorry to be getting back to you so late. Some of the vanilla JS alerts that I allude to are currently in these locations: These kinds of alerts are the ones that we want to update. The alerts that are shown above are React Errors which can now be documented in a separate issue for a future fix. |
Hi @GuillermoFloresV. Just so I have a good understanding of the updates, I should move on from the 3 errors about runtime and hydration with react. Instead, I need to focus on the alerts which show errors outputs upon calling the API endpoints. Rather than alerts, we would like them to be presented with a better UI, perhaps with an existing error component or by using tailwind classes. Sounds good? |
Yes, although not all alerts are caused by an error (see # 1 in my previous comment). Our main objective is to see an alert that lets the user know their request was successful/it failed. The examples I linked above are a starting point for you to get an idea, if those are finished we can move on to adding more where needed. We had previously installed a library known as react-toastify (There is no need for you to install it, it already exists in the repository) You can use this library as well to add the alerts we want. I can try to provide an example for you soon (I am currently working on refactoring the code for our |
Hey @GuillermoFloresV , I was not able to test the UI alerts for the student side as I could not see any classes to try to join. When I change my role to student, the dashboard is not visible and no classes are available to test to join. These following were untested: However, I did test on the teacher side and it is working. Here is the link to my commits: I know you are not suppose to make a PR without testing all the changes you made but I am not sure how to test from the student side. I was hoping you would be able to guide me. |
@Komal914 See https://dev.to/jmalvarez/how-to-cherry-pick-a-commit-from-another-repository-4pf1 @Komal914 My suggestion is for you to cherry-pick @GuillermoFloresV's pull request's 2 Git commits into @theGaryLarson's repository — especially if you believe that @GuillermoFloresV already fixed the underlying bug via the pull request at https://github.com/freeCodeCamp/classroom/pull/357/commits — the underlying bug that has been blocking you this week. Context: @Komal914 wrote at https://github.com/freeCodeCamp/classroom/pull/357#issuecomment-1636858282
|
Hi there, @Komal914 Sorry for getting back a bit late, the bug you are encountering shows that But I do not see you using this component inside your code, where did it come from? Sorry if I am missing some context. However, the underlying reason why you would not be able to see your toastify is because you are reloading the page before you call the component. So the browser starts a reload and then calls the component. I am working on this in #365 and I can push these changes to main to help you out if you have not been able to cherry-pick them yet, that is. Let me know and I can push them, I'll be finishing up that PR today either way so you should see it get merged relatively soon. |
Hi @GuillermoFloresV, I think the error you are referring to has been fixed (it was a typo). Currently I am struggling with the page reloading too quick before the alert has a chance to stay and notify. I will try to cherry pick just to learn the concept but I can also wait till a PR is up and pull in those changes. |
Status: @Komal914 confirmed that #357 is blocking this #134 On July 17th, Komal Kaur messaged Lloyd Chang, Guillermo Flores V in CodeDay Labs #help-desk channel at https://codedayorg.slack.com/archives/C05EZKUU456/p1689623550089979?thread_ts=1689278492.275899
Guillermo Flores V replied:
|
@Komal914 FWIW
Hence you can create a new pull request for #134 |
For Rob's mentees/micro-internship, do NOT assign to anyone else
Currently, the alerts we have set in place are vanilla JS alerts that can be seen as less user-friendly/cumbersome.
We want to be able to produce banner alerts or some other alert form that is more modern and sleek but still gets the overarching message across.
Perhaps something like this:
Current alert view:
The text was updated successfully, but these errors were encountered: