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

Add timeseries to region detail #9

Merged
merged 34 commits into from
Feb 21, 2024
Merged

Add timeseries to region detail #9

merged 34 commits into from
Feb 21, 2024

Conversation

Shane98c
Copy link
Member

@Shane98c Shane98c commented Feb 15, 2024

Main things here are making the timeseries graph into a reusable component and implementing the regionpicker/graphing the data!

Paired with carbonplan/maps#105

Some questions:

  • I'm coloring the lines based on the selected portion's average value... Would be cooler to implement a gradient sort of effect, but seems much more complex. Is the color worthwhile as is?
  • I threaded through the time horizon clipping, but we don't necessarily need to do that here and is maybe a bit odd since that filter is not accessible here currently. I'm also clipped the available years in the slider for consistency.
  • maybe we should get some react-spring magic going here for the axis changes/point locations?
  • should we add an interaction where you can click the line and that would set the elapsedTime?
  • loading state definitely needed but maybe that's a different pr?
  • seeing some footer jank (so far just in Arc on the vercel preview) when opening the timeseries for the first time - are you also noticing that?

Copy link

vercel bot commented Feb 15, 2024

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

Name Status Preview Comments Updated (UTC)
oae-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 21, 2024 4:14pm

Copy link
Member

@katamartin katamartin left a comment

Choose a reason for hiding this comment

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

So cool to see this wired up, the tool already feels so much richer!

On your questions:

  • I'm coloring the lines based on the selected portion's average value... Would be cooler to implement a gradient sort of effect, but seems much more complex. Is the color worthwhile as is?

    • I like the color as-is! But the gradient idea is a cool one -- maybe something to explore as a follow-up?
  • I threaded through the time horizon clipping, but we don't necessarily need to do that here and is maybe a bit odd since that filter is not accessible here currently. I'm also clipped the available years in the slider for consistency.

    • I don't feel very strongly either way on this one, but I did notice a couple of small bugs with the current clipping behavior:
      • open region details => set elapsedTime=15 years => close region details => set timeHorizon=10 years => open region details (current: elapsedTime=15 years, expected: elapsedTime=0 OR elaspedTime=10 years
      • because we're rendering the time series with two different Lines, there's a gap around the timeHorizon. This is especially noticeable in the regional time series, but also present in the overview time series.
      • CleanShot 2024-02-16 at 10 05 35@2x
  • maybe we should get some react-spring magic going here for the axis changes/point locations?

    • Yeah I think that'd be great for the axis changes! For the point locations, the lines, and the Rects, we could consider just using a basic CSS transition. I think that got us pretty far in animating the year-to-year changes in the CMIP6 time series for monthly data.
  • should we add an interaction where you can click the line and that would set the elapsedTime?

    • Addressed inline in my comment about regionOptions
  • loading state definitely needed but maybe that's a different pr?

    • 👍 different PR sounds good to me
  • seeing some footer jank (so far just in Arc on the vercel preview) when opening the timeseries for the first time - are you also noticing that?

    • Hmm I haven't yet noticed that, but I can try testing some more.


const handleMonthChange = useCallback(
(month) => {
const years = Math.floor(elapsedTime / 12)
setElapsedTime(years + month)
setElapsedTime(years * 12 + month)
Copy link
Member

Choose a reason for hiding this comment

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

oops, good catch!

Comment on lines +53 to +56
selector: {
polygon_id: selectedRegion,
injection_date: injectionDate,
},
Copy link
Member

Choose a reason for hiding this comment

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

Curious if you thought about the tradeoffs of showing the entire time series through the end of the timeHorizon vs. showing seasonal trends one year at a time (i.e. one chunk at a time).

I like that you can quickly glimpse the behavior over all time with the current behavior, and this seems especially useful in the context of a Download CSV button!

But in the context of this potential interaction:

should we add an interaction where you can click the line and that would set the elapsedTime?

which I think feels like a pretty natural one, the yearly data might work better (at least with the current set of time sliders). In that world, the month slider would just change the where the Circle is rendered in the time series and the year slider would allow you to view changes in seasonal trends over time.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

My sense is that the most interesting overall trends will be over the years, with the seasonal variability being a less significant aspect (could certainly be wrong here), so showing the full run through the years seems most insightful to me. I think not having the the full data visible all at once would make finding those longer scale trends much harder (they seem pretty subtle from what I've been able to see exploring around so far).

I just pushed a version where clicking on the line sets the elapsed time to that point (or at least the closest datapoint to the click). I think adding a vertical line that follows the mouse and letting people click anywhere on the graph might be a better solution but wanted to test this first!

Copy link
Member

Choose a reason for hiding this comment

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

On the time series extent: this version feels like the most clear "overview," so happy with what we have for now! As the store2 array grows in size, we should just keep an eye on how much data we're fetching to render this whole thing at once. Also, might be good to check in on this question when we share the prototype out.

On the the elapsed_time selection: I like the line treatment a lot! Could be nice to add a little label at the top that reads OCT YEAR 1 or similar? The delta between the click point at the closest datapoint can feel a little jumpy in some cases (see below). Does it feel choppy to just render the line over the elapsed_time step nearest to the mouse?
CleanShot 2024-02-20 at 10 23 26@2x

components/timeseries.js Outdated Show resolved Hide resolved
const [mousePosition, setMousePosition] = useState(null)
const [isHovering, setIsHovering] = useState(false)

const animatedYLimits = useSpring({
Copy link
Member

Choose a reason for hiding this comment

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

Okay on viewing it, I actually find this animation rather disorienting haha. What do you think about dropping the animation on the Chart and just using CSS transitions for the Plot elements (as mentioned in this comment)

For the point locations, the lines, and the Rects, we could consider just using a basic CSS transition. I think that got us pretty far in animating the year-to-year changes in the CMIP6 time series for monthly data.

@Shane98c Shane98c merged commit 2d941ae into main Feb 21, 2024
2 checks passed
@Shane98c Shane98c deleted the detail-timeseries branch February 21, 2024 17:30
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