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

feat: ranking panel rework #6867

Merged
merged 38 commits into from
Aug 1, 2024
Merged

feat: ranking panel rework #6867

merged 38 commits into from
Aug 1, 2024

Conversation

VIKTORVAV99
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 commented Jun 19, 2024

Issue

Closes: AVO-17
Closes: AVO-337
Closes: #4530

Description

Link to Figma: https://www.figma.com/design/O1DtS6UyZHSH4IymG8Ufsw/%F0%9F%8E%B9-Design-Hack?node-id=4195-2114&t=G7O58Pa8jUXOwk6y-1

Preview

Double check

  • I have tested my parser changes locally with poetry run test_parser "zone_key"
  • I have run pnpx prettier@2 --write . and poetry run format in the top level directory to format my changes.

@VIKTORVAV99 VIKTORVAV99 changed the title wip: ranking panel rework feat: ranking panel rework Jul 30, 2024
@tonypls
Copy link
Collaborator

tonypls commented Jul 31, 2024

The padding for each of these lines would be nice if it was equal. Slightly more than the top padding is currently:
Screenshot 2024-07-31 at 10 53 44 AM

Optional fix: iPad mini or smaller desktop size the buttons are not completely visible
image

Will also do a code review now (and ask the pr agent to do one 🤖)

/review

@tonypls
Copy link
Collaborator

tonypls commented Jul 31, 2024

/review

Copy link
Contributor

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review

Possible Bug
The new isOnTop prop is added to control the direction of the chevron icon. However, the logic seems inverted in the implementation. When isOnTop is true, HiChevronUp is shown when the accordion is collapsed, which is counterintuitive as it suggests the accordion should open upwards.

API Change
The disabled prop has been renamed to isDisabled. This is a breaking change and all instances in the project where Button is used must be updated to reflect this change.

Redundant Code
The InfoModalContent function includes a div structure that seems to be left from the previous implementation. This could be cleaned up to better align with the new button components used.

@VIKTORVAV99
Copy link
Member Author

The padding for each of these lines would be nice if it was equal. Slightly more than the top padding is currently: Screenshot 2024-07-31 at 10 53 44 AM

Optional fix: iPad mini or smaller desktop size the buttons are not completely visible image

Will also do a code review now (and ask the pr agent to do one 🤖)

/review

Overlap should be fixed or mitigated by #7024 which was originally part of #6911.

I'll look into the padding and double check it with Alex as well. 👍🏻

@VIKTORVAV99
Copy link
Member Author

Fixed the margins and hight so it looks more uniform but a more permanent responsive fix requires #7020 or something similar.

Possibly also some responsive tweaks about the header as it looks very strange on say a iPad at the moment (when visiting the website).

Copy link
Collaborator

@tonypls tonypls left a comment

Choose a reason for hiding this comment

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

Couple of small comments but overall loooks great!

Feel free to scrutinize my padding changes, my goal was to have no padding on the Outer left panel section, reduce the padding on the new stuff to make it maximize ranking space and just meet the top of the Historical Slider component.

web/src/components/Accordion.tsx Outdated Show resolved Hide resolved
web/src/features/modals/InfoModal.tsx Outdated Show resolved Hide resolved
</a>
<div className="flex basis-0 gap-x-4">
<PrivacyPolicyButton />
<VerticalDivider />
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is very hard to see in both light and dark mode

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll adjust it slightly but it's not meant to be super visible. Let me know what you think about the new version.

web/src/features/modals/InfoText.tsx Show resolved Hide resolved
@tonypls
Copy link
Collaborator

tonypls commented Aug 1, 2024

Do you know where this border on the input came from? It appeared after my commit but I can't figure out how?!
image

@VIKTORVAV99
Copy link
Member Author

VIKTORVAV99 commented Aug 1, 2024

Do you know where this border on the input came from? It appeared after my commit but I can't figure out how?! image

It has appeared in other PRs as well, best I can think of is some sort of dependency change that happened earlier today (Dependabot updates all dependencies the 1st of each month) but I'll look into why it's happening.

It has nothing to do with this PR though.

EDIT: It's not affected on staging so probably not a dependency change but it's also affecting #7048 so I don't really know what the issue is. Will try and dig into it a bit.

@tonypls
Copy link
Collaborator

tonypls commented Aug 1, 2024

Do you know where this border on the input came from? It appeared after my commit but I can't figure out how?! image

It has appeared in other PRs as well, best I can think of is some sort of dependency change that happened earlier today (Dependabot updates all dependencies the 1st of each month) but I'll look into why it's happening.

It has nothing to do with this PR though.

EDIT: It's not affected on staging so probably not a dependency change but it's also affecting #7048 so I don't really know what the issue is. Will try and dig into it a bit.

I'm very confused because it's doesn't happen with this branch locally but it does on the preview 🤔

Copy link
Collaborator

@tonypls tonypls left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

Looks great, glad to get this one out there :)

@VIKTORVAV99
Copy link
Member Author

VIKTORVAV99 commented Aug 1, 2024

Do you know where this border on the input came from? It appeared after my commit but I can't figure out how?! image

It has appeared in other PRs as well, best I can think of is some sort of dependency change that happened earlier today (Dependabot updates all dependencies the 1st of each month) but I'll look into why it's happening.
It has nothing to do with this PR though.
EDIT: It's not affected on staging so probably not a dependency change but it's also affecting #7048 so I don't really know what the issue is. Will try and dig into it a bit.

I'm very confused because it's doesn't happen with this branch locally but it does on the preview 🤔

If you try and reinstall your dependencies does it show up then? Maybe it's a dependency thing anyway (I forgot that I now have to update staging as well so maybe it's there anyway).

EDIT: I think I tracked it down to this PR #7035

Will revert the dependencies one by one locally and find out which one it is and figure out if it's a bug or we had a bug that are now "fixed".

@VIKTORVAV99 VIKTORVAV99 merged commit c47bf6c into master Aug 1, 2024
21 checks passed
@VIKTORVAV99 VIKTORVAV99 deleted the vik/ranking_panel_rework branch August 1, 2024 20:13
@VIKTORVAV99
Copy link
Member Author

Narrowed down the problem to this PR tailwindlabs/tailwindcss-forms#141 of @tailwindcss/forms which was included in version 0.5.4.

I'll create a PR to revert the package here and then open an issue with them describing the issue.

And if this is the intended behaviour (we were not specifying a type) then I'll create a pr that removes this styling instead and add type='search to the input for accessibility purposes.

@tonypls
Copy link
Collaborator

tonypls commented Aug 2, 2024

Narrowed down the problem to this PR tailwindlabs/tailwindcss-forms#141 of @tailwindcss/forms which was included in version 0.5.4.

I'll create a PR to revert the package here and then open an issue with them describing the issue.

And if this is the intended behaviour (we were not specifying a type) then I'll create a pr that removes this styling instead and add type='search to the input for accessibility purposes.

Nice investigating! Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Colourblind mode checkbox/info panel obstructs region list on short desktop screens or when zoomed in
4 participants