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

Fix ratings columns on smaller screens #279

Merged

Conversation

jauggy
Copy link
Member

@jauggy jauggy commented Apr 21, 2024

Currently in production it looks like this for small screens:
1

This ticket gives a better layout for smaller screens by adjusting the column widths and padding. Also I changed the uncertainty delta color to grey because it will always go down and users might be confused why that column is always red (and it's not bad to go down).
2

Test Steps

Go here http://localhost:4000/battle/ratings

Test on resolutions such as
1360 x 768
1280 x 1024
(or similar if you don't have any of these screen sizes)

Fix ratings columns
@jauggy jauggy changed the title WIP: Fix ratings columns Fix ratings columns Apr 21, 2024
@jauggy jauggy changed the title Fix ratings columns Fix ratings columns on smaller screens Apr 21, 2024
@jauggy jauggy marked this pull request as ready for review April 21, 2024 11:02
@jauggy
Copy link
Member Author

jauggy commented Apr 21, 2024

@L-e-x-o-n can you please review if you have spare time thanks.

@L-e-x-o-n
Copy link
Collaborator

L-e-x-o-n commented Apr 22, 2024

1360x768 and 1280 x 1024

Should uncertainty arrow should always be next to number (not in next row)?

PR

image

Original 1360x768 and 1280 x 1024

image

1920x1080

For reference, looks the same with and without PR, except grey uncertainty
image

@jauggy
Copy link
Member Author

jauggy commented Apr 22, 2024

Yeah your screenshot looks wrong. I'll take a look.

@jauggy
Copy link
Member Author

jauggy commented Apr 22, 2024

@L-e-x-o-n can you try in incognito mode. Maybe css is getting cached? Also please check you have the latest from this pr/branch. In my PR, each row should only take up one line and the arrow should be on same row.

@L-e-x-o-n
Copy link
Collaborator

Tried, on latest PR, same thing :( Firefox

@jauggy
Copy link
Member Author

jauggy commented Apr 22, 2024

I tested with Firefox again (latest version 125.0.2 and it works). can you do an experiment for me?

Open up
assets/scss/custom/_styles.scss and modify to add background-color to the Map column:
Find this section

//Map column for Ratings table
.table-ratings tr th:nth-child(1) {
  width: 200px;
}

Then modify to this:

//Map column for Ratings table
.table-ratings tr th:nth-child(1) {
  width: 200px;
  background-color: red;
}

Do you get the new color for the map heading?

@L-e-x-o-n
Copy link
Collaborator

I tested with Firefox again (latest version 125.0.2 and it works). can you do an experiment for me?

Open up assets/scss/custom/_styles.scss and modify to add background-color to the Map column: Find this section

//Map column for Ratings table
.table-ratings tr th:nth-child(1) {
  width: 200px;
}

Then modify to this:

//Map column for Ratings table
.table-ratings tr th:nth-child(1) {
  width: 200px;
  background-color: red;
}

Do you get the new color for the map heading?

Doesn't work, even with cleared cache...

@jauggy
Copy link
Member Author

jauggy commented Apr 22, 2024

Part of the local setup is sass install

mix sass.install

@L-e-x-o-n Can you run that and try again?

@L-e-x-o-n
Copy link
Collaborator

L-e-x-o-n commented Apr 23, 2024

Part of the local setup is sass install

mix sass.install

@L-e-x-o-n Can you run that and try again?

No changed :(
Would be nice if someone else can try, maybe it's just me?

@jauggy
Copy link
Member Author

jauggy commented Apr 23, 2024

geekingfrog can you help test this PR? If the css is working correctly you should be able to go here:
http://localhost:4000/battle/ratings
and the columns will all be same width except the first one. For some reason it's not working for Lexon.

EDIT: Nevermind geekingfrog; Adam has tested it below.

@AdamChlupacek
Copy link
Contributor

I can confirm the PR works as expected:

On master:
Screenshot 2024-04-23 at 15 27 09

On your branch:
Screenshot 2024-04-23 at 15 27 49

This is working as expected.

@jauggy
Copy link
Member Author

jauggy commented Apr 23, 2024

Thanks Adam. I wonder why css is not working on Lexon's machine.

@geekingfrog
Copy link
Contributor

It breaks down under 1080px for viewport width, but otherwise it's a net improvement, can fix that (mobile view?) later.


//Map column for Ratings table
.table-ratings tr th:nth-child(1) {
width: 200px;
Copy link
Contributor

Choose a reason for hiding this comment

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

not a fan of putting dimensions in pixels, since they don't adapt with font size. So if the user changes its font size, it gets out of whack easily. The default font size is 16px, that means 200px is 12.5rem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Have updated to use REM and confirmed it looks the same.

@StanczakDominik StanczakDominik merged commit c0a4773 into beyond-all-reason:master May 28, 2024
1 check failed
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.

5 participants