Skip to content

Ux vote loading#74

Merged
huonggiangbui merged 8 commits intoutmgdsc:dev/gdsc-open-source-2022from
ggggg:UX-vote-loading
Dec 28, 2022
Merged

Ux vote loading#74
huonggiangbui merged 8 commits intoutmgdsc:dev/gdsc-open-source-2022from
ggggg:UX-vote-loading

Conversation

@ggggg
Copy link
Collaborator

@ggggg ggggg commented Oct 10, 2022

  • Added a loading wheel for when the user clicks on a vote until the server responds.
  • If the server doesn't respond than display error message (see image below)

Fixes: #73

Screenshots and screen captures:
image
image

Self-review checklist

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review:

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes (what you are trying to accomplish? e.g. don't let user's age less than 0).

Completed manual review and testing of the following (please attach screenshots/gifs for UI changes and commit your test cases for functionality changes):

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

Copy link
Member

@huonggiangbui huonggiangbui 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 to me!

@huonggiangbui
Copy link
Member

huonggiangbui commented Oct 10, 2022

I also request another project owner to take a look, because I'm not sure if they want to add another library (Material UI)

Copy link

@embeddedt embeddedt left a comment

Choose a reason for hiding this comment

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

The only thing I don't like about this (and this might be subjective) is that it adds MUI as a dependency. My experience with MUI in another fairly large project is that it slows down TypeScript a LOT and makes autocompletion very painful to use (because of their styling system).

@ggggg
Copy link
Collaborator Author

ggggg commented Oct 10, 2022

I cannot request anyone to review, only maintainers can. It was not my idea to add MUI, @huonggiangbui requested it in #73

@huonggiangbui
Copy link
Member

huonggiangbui commented Oct 10, 2022

LOL you can use any library you want, MUI is just a suggestion

@huonggiangbui
Copy link
Member

MUI is fine imo, but I want to see what Shubh or Chris think about this

@ggggg
Copy link
Collaborator Author

ggggg commented Oct 10, 2022

LOL you can use any library you want, MUI is just a suggestion

Oh, sorry for the misunderstanding.

@huonggiangbui
Copy link
Member

It's okay, I should make it more clear!

@hiimchrislim
Copy link
Member

hiimchrislim commented Oct 11, 2022

Personally, I wouldn't add in MUI. We already have Tailwind CSS and personally, I don't want to be adding in another dependency for this. For example: https://flowbite.com/docs/components/spinner/ - Just found this after a quick google search for a spinner using Tailwind

@hiimchrislim hiimchrislim self-requested a review October 11, 2022 06:54
@ggggg
Copy link
Collaborator Author

ggggg commented Oct 11, 2022

Personally, I wouldn't add in MUI. We already have Tailwind CSS and personally, I don't want to be adding in another dependency for this. For example: https://flowbite.com/docs/components/spinner/ - Just found this after a quick google search for a spinner using Tailwind

The spinner wasn't what we added MUI for, we wanted a cleaner error message. But I'll come up with something.

@ggggg
Copy link
Collaborator Author

ggggg commented Oct 12, 2022

Used alert() because this message shouldn't pop up often and to avoid adding another library/component. Please review and let me know of any changes you want me to make.

@logonoff
Copy link

logonoff commented Oct 12, 2022

If tailwind is already a dependency a better solution that might look nicer is to use one of their modal components https://tailwindui.com/components/application-ui/overlays/modals

@ggggg
Copy link
Collaborator Author

ggggg commented Oct 12, 2022

If tailwind is already a dependency a better solution might look nicer to use one of their modal components https://tailwindui.com/components/application-ui/overlays/modals

I will look into it, thanks!

@ggggg
Copy link
Collaborator Author

ggggg commented Oct 22, 2022

@logonoff Sorry for this taking a while, I had midterms and was busy with other school stuff.
Anyways, this should now use tailwind's alert box instead of the ugly javascript one.
Here is a gif preview:
gif_preview

@embeddedt
Copy link

embeddedt commented Oct 23, 2022

I know this is incredibly picky, but I'm not liking that the spinner isn't centered around the C. 😆 Is it easy to fix that?

The alert box looks great!

@ggggg
Copy link
Collaborator Author

ggggg commented Oct 23, 2022

Well the spinner is centered, I think anything else would look odd. A possible fix can be to mess around with the size of the spinner.

@ggggg
Copy link
Collaborator Author

ggggg commented Oct 23, 2022

Are any of these any better @embeddedt?
chrome_spNEvwLG2X
chrome_X0FsL5nITg

@embeddedt
Copy link

embeddedt commented Oct 23, 2022

The issue is less obvious now but it's still not centered compared to the center of the C. I'm not sure why. It's weird, since I assume the buttons have centered text and the spinner is also centered.

@embeddedt
Copy link

Also, I like the first design in #74 (comment) the best (the one with no background), as it looks cleaner IMO and should also be easier to port to a future dark mode.

ggggg and others added 2 commits October 23, 2022 19:14
Thanks @embeddedt

Co-authored-by: embeddedt <42941056+embeddedt@users.noreply.github.com>
@huonggiangbui huonggiangbui requested review from hiimchrislim and shubhbapna and removed request for hiimchrislim and shubhbapna October 24, 2022 03:59
@ggggg
Copy link
Collaborator Author

ggggg commented Dec 23, 2022

Hey is this still open?

Copy link

@logonoff logonoff left a comment

Choose a reason for hiding this comment

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

looks fine to me

Copy link
Collaborator

@shubhbapna shubhbapna left a comment

Choose a reason for hiding this comment

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

LGTM

@huonggiangbui huonggiangbui merged commit 7d91038 into utmgdsc:dev/gdsc-open-source-2022 Dec 28, 2022
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.

6 participants