Fix rating review overflow #545
Conversation
charlieweinberger
temporarily deployed
to
staging-545
GitHub Actions
Inactive
There was a problem hiding this comment.
most of my comments are about the same concept, but to put it visually, these are two rating tiles that have the same width... it would be better to have everything conform to a fixed width instead of having some parts of it depend on the user's view port width (i.e. why is the rating text bigger on one?)
| width: 5vw; | ||
| height: 5vw; |
There was a problem hiding this comment.
Change this to use a fixed width instead of vw
There was a problem hiding this comment.
Having a fixed height also makes it so you only have to check a couple different screen sizes (i.e. phone, desktop) instead of checking the whole continuous range
| min-width: 7rem; | ||
| min-height: 7rem; |
There was a problem hiding this comment.
If you have a fixed height, you can remove this
| .rating { | ||
| width: 8vw; // The rating box width & height shrink... | ||
| height: 8vw; | ||
| min-width: 5rem; // ...but only by so much. | ||
| min-height: 5rem; | ||
| gap: max(0.5vw, 0.4rem); // The same idea goes for gap... | ||
| } |
There was a problem hiding this comment.
This can be a lot less confusing if we have a fixed width for the rating tiles. It's common to think that things should scale with screen size (and that was the assumption before this PR), but most of the time that's not really true.
| .rating { | ||
| width: 45%; | ||
| height: 20vw; | ||
| } |
There was a problem hiding this comment.
Another rule that can be removed if we simplify the rating tile sizing
charlieweinberger
temporarily deployed
to
staging-545
GitHub Actions
Inactive
…th fixed widths, and fixed the remaining responsiveness issues
charlieweinberger
temporarily deployed
to
staging-545
GitHub Actions
Inactive
|
After doing further work on improving the UI of reviews, I've realized that the code I pushed a few hours ago isn't ready for prod. I'm working on a fully-functional version right now, which will also have a bunch of other improvements to the UI of reviews. Unfortuately, I got a little carried away and have changed a lot more than just fixing this issue, althought I still believe that I've simply solved a number of other bugs related to the UI/UX of reviews. Within the next day I'll update this pull request with all the changes, and you can be the judge. |
|
I've realized that its likely easier to create a new pull request, because of how much I changed and in the interest of preserving this pull request in case my new pull request is too much. The new pull request is #552: Improve review UI. Go check that out and respond over there. I'm really sorry if this entire situation is problematic. I don't know a lot about the good practices and typical norms of pull requests / team collaboration, so I apologize if I'm causing a mess here. |
Fixed the overflow of the "quality"/"difficulty" headers in ratings for professor/course reviews.
Description
Wherever reviews can be, the rating headers (and actually the rating values and the colored box itself) were not properly resizing when the browser window changed size. I fixed this by adding a media query for between widths of 1300px and 600px (there were no issues for widths outside of that range), and adjusting the widths and heights of the rating box, rating label, and rating value. I also fixed a bug where the rating boxes weren't the correct sizes at exactly 600px wide, by updating the smaller media query to start at 599px instead of 600px.
I definitely applied some of my personal opinions to this fix, since there isn't one clear way to make this section look the best. Let me know what you guys think about my changes, and if I should make the elements react different when the window changes dimensions.
Screenshots
Before:
After:
Test Plan
Create a review, then change the dimensions of the browser window, observing the ratings at various dimensions and ensuring that the rating header both stays inside the box the entire time, and looks good in terms of spacing & sizing for all window sizes. Make sure to check different widths, different heights, and both. Focus on widths between 1300px and 600px.
Issues
Closes #304