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

TTWWW-475: Prevalence Map timeline styling #50

Open
wants to merge 7 commits into
base: mike/TTWWW-468
Choose a base branch
from

Conversation

mattga11ant
Copy link
Collaborator

@mattga11ant mattga11ant commented Feb 10, 2025

For task: https://simonsfoundation.atlassian.net/browse/TTWWW-475

Because the matt/TTWWW-471 branch contained everything, I used that as the starting point for this 475 task. Let me know if I should have done this differently.

In Figma in the area showing the breakpoints. The timeline has more left padding between the "Timeline of studies" copy on the left, and the timeline itself. On the higher breakpoints, this padding is on the right. It didn't seem intended that there was a larger gap between these two items only on the small breakpoint. So I made the padding consistent there. I attached a screenshot showing this.

Some clarification I will need here. If I hover a dot on the map, does the tooltip show on the map and the timeline? And then if I hover the dot on the timeline, does that show on the map? Does clicking do the same thing? Current functionality, when you hover a dot on the map, you don't get a tooltip on the map. And we have already seen differences in Figma. So not sure what is wanted here. Currently I have it where if one is hovered or clicked, the other does the same thing.

timelinereference
  • check out this branch
  • compile front-end updates using npm run dev
  • confirm that the timeline looks correct for 1280px wide, 1440px wide, and 1728px wide. Comparing to Figma. The 1280px wide version is the difference I mention above.
  • confirm that the Year published / Year(s) studied slider works when clicking it
  • confirm that the help tooltip appears when you click the ? icon
  • confirm that dragging your cursor across the timeline produces the limiting box where a user can control what years are shown. This box should have the red dashed outline, red dot indicators, and white labels as shown in Figma.
  • confirm that the "clear" button clears the timeline selection
  • confirm that when hovering a dot on the timeline, that dot appears white and shows the tooltip above the dot
  • confirm that hovering a dot on the timeline shows the related dot on the map
  • confirm that clicking a dot on the timeline shows the related dot on the map, and clicking again removes them

@yjfnyc yjfnyc requested review from yjfnyc and removed request for jessica-bee February 12, 2025 17:33
@yjfnyc
Copy link

yjfnyc commented Feb 12, 2025

Hi @mattga11ant,

I just did a brief look at the functionality, seems to work well. I only have one issue which is the dots stack partially over the one below it as shown on the screenshot below.
Screenshot 2025-02-12 at 12 36 59 PM
As to your questions, for the tool tip one, I did notice the difference between the design and the implementation, I just wanted to leave it to the QA people. As to the spacing one, we can also do the same.

I would also like you to resolve the conflicts of this PR before I can really take a look at the changed code. Thanks, Matt!

@mattga11ant
Copy link
Collaborator Author

mattga11ant commented Feb 12, 2025 via email

@yjfnyc
Copy link

yjfnyc commented Feb 12, 2025

The dots stacking is necessary functionality. If we were to always have them stack exactly like they show in the design, then there would be a finite number of dots that could fit. If there are more dots than there is space, they stack like that. It is the functionality we see on the current site as well. I am open to suggestions if you know of a way around this. For the merge conflicts, I generally deal with those when merging. How would you like me to handle those before we merge?

On Wed, Feb 12, 2025 at 11:24 AM Jinfeng Ye @.***> wrote: Hi @mattga11ant https://github.com/mattga11ant, I just did a brief look at the functionality, seems to work well. I only have one issue which is the dots stack partially over the one below it as shown on the screenshot below. Screenshot.2025-02-12.at.12.36.59.PM.png (view on web) https://github.com/user-attachments/assets/eeb45940-fb99-4971-b193-cdb02b06b135 As to your questions, for the tool tip one, I did notice the difference between the design and the implementation, I just wanted to leave it to the QA people. As to the spacing one, we can also do the same. I would also like you to resolve the conflicts of this PR before I can really take a look at the changed code. Thanks, Matt! — Reply to this email directly, view it on GitHub <#50 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAT5KRT4JUERWMN3F267AWL2POGVXAVCNFSM6AAAAABW2XI5YOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNJUGUZDCNBRGQ . You are receiving this because you were mentioned.Message ID: </pull/50/c2654521414@ github.com>

Hi @mattga11ant , Thanks for the quick response. I just tested on the current site narrowing the timeline to the same height as the design, and it did exhibit stacking, so you were right. So I will take this stacking behavior of dots as legit.

As to the conflicts, that has to do with this branch being based on the old ttwww-471, I am kind of concerned if there is any back and forth happening here in code changing, it would be not that easy to tell which is which. So resolving the conflict before code review would hopefully make that easier I think.

As to how to resolve it, I think you can merge the epic branch into this feature branch locally, or you can resolve conflicts on github, which I guess would do the same.

@mattga11ant
Copy link
Collaborator Author

@yjfnyc I have fixed the merge conflicts. This is ready for your review.

@mattga11ant
Copy link
Collaborator Author

@yjfnyc didn't realize there were additional merge conflicts, and that is likely why you had not finished this review. I have those corrected now.

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.

2 participants