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: Fixed tooltip for Mobile Area Charts #1817

Merged
merged 11 commits into from
Nov 11, 2024

Conversation

noahonyejese
Copy link
Contributor

@noahonyejese noahonyejese commented Nov 6, 2024

@bprusinowski please check if this is the correct approach, if so I will continue implementing this for all charts and for tooltips that are larger the arrow will have to be more dynamic of course.

This PR:

  • Fixes Mobile Tooltip for all Chart except (Pie Charts, Scatterplot Charts, Map Charts)
  • The exceptions still need design criteria, @KerstinFaye
  • This PR also uncovered a new Issue #110
  • This PR is the start to fix Issue #94

This PR will resolve #94


How to Test

  1. Go to this Visit Preview

  2. Use the Einmalvergütung für Photovoltaikanlagen Dataset (you can test almost all Chart types w/ this)

  3. Select Bar Chart -> Publish -> See how the tooltip is at the bottom with adjusting arrow ✅

  4. Select Line Chart -> Publish -> See how the tooltip is at the bottom with adjusting arrow ✅

  5. Select Area Chart -> Publish -> See how the tooltip is at the bottom with adjusting arrow ✅

  6. Select Scatter Chart -> Publish -> See how the tooltip is at the bottom with adjusting arrow, but not optimal design ✅

  7. Select Pie Chart -> Publish -> See how the tooltip is not placed correctly on mobile (potential separate issue), but is highlighted correctly ✅

  8. Select Dual Axis Line Chart -> Publish -> See how the tooltip is at the bottom with adjusting arrow, but not optimal design ✅

  9. Select Column Line Chart -> Publish -> See how the tooltip is at the bottom with adjusting arrow ✅


CC: @KerstinFaye , @sosiology

Copy link

vercel bot commented Nov 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
visualization-tool ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 11, 2024 6:07am

Copy link
Collaborator

@bprusinowski bprusinowski left a comment

Choose a reason for hiding this comment

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

The overall approach looks good to me, thank you @noahonyejese! There's another problem that probably was there before, that the tooltip can go out of the viewport – I wonder if we maybe can take a look at the same time?

Screenshot 2024-11-06 at 14 42 35

app/charts/area/areas-state.tsx Outdated Show resolved Hide resolved
app/charts/area/areas-state.tsx Outdated Show resolved Hide resolved
@noahonyejese
Copy link
Contributor Author

@bprusinowski yeah I think the issue you mentioned above will resolve once we implement dynamic arrows

@bprusinowski
Copy link
Collaborator

Ah, but here it's not about the arrow, but that the whole tooltip can go out of viewport 👀 But maybe I missed something 👍

@noahonyejese
Copy link
Contributor Author

Ah, but here it's not about the arrow, but that the whole tooltip can go out of viewport 👀 But maybe I missed something 👍

yeah technically good point its kinda both, where when we move the tooltip the arrow will be off and vise versa I will make sure it works once this is done

Note: This doesn't include the Map and Pie Chart yet.
@noahonyejese
Copy link
Contributor Author

@bprusinowski I think everything works here, except for the Pie Chart, Map Chart and Scatter Plot. I will be updating the description for testing

Copy link
Collaborator

@bprusinowski bprusinowski left a comment

Choose a reason for hiding this comment

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

Thanks @noahonyejese, LGTM 👏 Just a few nit comments 👍

app/utils/use-is-mobile.ts Show resolved Hide resolved
app/charts/shared/interaction/tooltip-box.tsx Outdated Show resolved Hide resolved
app/charts/shared/interaction/tooltip-box.tsx Outdated Show resolved Hide resolved
app/charts/shared/interaction/tooltip-box.tsx Outdated Show resolved Hide resolved
app/charts/shared/interaction/tooltip-box.tsx Outdated Show resolved Hide resolved
app/charts/shared/interaction/tooltip-box.tsx Outdated Show resolved Hide resolved
app/charts/line/lines-state.tsx Outdated Show resolved Hide resolved
app/charts/combo/combo-line-dual-state.tsx Outdated Show resolved Hide resolved
app/charts/shared/interaction/tooltip-box.tsx Outdated Show resolved Hide resolved
@noahonyejese
Copy link
Contributor Author

noahonyejese commented Nov 11, 2024

@sosiology , @KerstinFaye this is ready for testing please free to compare everything w/ the designs as well:


Design References:

  • Scatterplot Chart: Added a ruler, but no highlighting. Position on Top if multiple Datapoints are in one line.
    Visual Design Scatterplot

  • Pie Chart: Added Highlight (border line). Position of the tooltip would stay in place.
    Visual Design Pie Chart

  • Map Chart: Highlight is already implemented. Would keep this, but ruler not necessary since position of the Tooltip would stay in place.
    Visual Design Map

@noahonyejese noahonyejese merged commit 48d26db into main Nov 11, 2024
5 of 6 checks passed
@noahonyejese noahonyejese deleted the fix/mobile-tooltip-positioning branch November 11, 2024 10:05
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.

Data Filter after Publication - unexpected error
2 participants