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

Redesign platform indicators on dispatch board #418

Merged
merged 8 commits into from
Jan 24, 2025

Conversation

evertiro
Copy link

@evertiro evertiro commented Jan 7, 2025

Rework platform indicators into a cleaner, simpler design. Note that there is a preexisting error with linting that has not been resolved due to the ps and xb classes being too short per our rules. Wasn't sure how to deal with this and since it's preexisting, I chose to leave it alone for now.

Fixes #413

Dan Griffiths added 2 commits January 7, 2025 16:02
Copy link
Member

@UncleClapton UncleClapton left a comment

Choose a reason for hiding this comment

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

just got a couple things here

src/components/DispatchTable/RescueRow.js Show resolved Hide resolved
Comment on lines 96 to 114
&.horizons3 > .platformBadgeLabel {
background-color: $magenta-dark3;
}

&.horizons4 {
&.horizons4 > .platformBadgeLabel {
background-color: $blue-dark3;
}

&.odyssey {
&.odyssey > .platformBadgeLabel {
background-color: $gold-dark2;
}
}
}

.platform {
&.ps {
color: $playstation-blue-light3
}
&.ps > .platformBadgeLabel {
background-color: $xbox-green-light;
}

&.xb {
color: $xbox-green-light
}
&.xb > .platformBadgeLabel {
background-color: $playstation-blue-light3;
}
Copy link
Member

@UncleClapton UncleClapton Jan 9, 2025

Choose a reason for hiding this comment

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

We should avoid useless hoisting of modifier classes like this. I'd say it would be better for these to exist as modifiers within the platformBadgeLabel class instead

Ideally we'd also try to keep our style structure within CSS modules as flat a possible, as they all complile to scoped CSS names anyway. There's no need to hold a definite element structure when the compiler assures all resulting class names will be unique. I believe this file is especially bad in that regard simply because it was faster to copy&paste the old stylesheet instead of recreating it for modules 😂. No need to do a big cleanup right now if you don't want to, but just wanted to call it out cuz it's easy to paint yourself into a corner by strictly defining your styles in this way.

Copy link
Member

Choose a reason for hiding this comment

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

@evertiro this was left unaddressed 🙂

Dan Griffiths added 2 commits January 8, 2025 19:53
@evertiro evertiro requested a review from UncleClapton January 11, 2025 11:29
Dan Griffiths added 4 commits January 11, 2025 05:41
Signed-off-by: Dan Griffiths <[email protected]>
This reverts commit 56b23a4.
Signed-off-by: Dan Griffiths <[email protected]>
Copy link
Member

@UncleClapton UncleClapton left a comment

Choose a reason for hiding this comment

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

LGTM, Awesome work :)

@UncleClapton UncleClapton merged commit c89465d into FuelRats:develop Jan 24, 2025
2 checks passed
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.

Change style of platform indicators in dispatch board
2 participants