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

Compare line page #1351

Open
wants to merge 18 commits into
base: development
Choose a base branch
from

Conversation

nmqng
Copy link

@nmqng nmqng commented Oct 3, 2024

Description

The purpose of this pull request is to add a comparison line graphic page based on the design doc.

Fixes #953

Type of change

  • Note merging this changes the database configuration.
  • This change requires a documentation update

Checklist

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request
  • You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

Limitations

Currently, when a shifting to/from a leap year from/to a non-leap year, a warning show up to tell the user that the original graph and the shifted graph will not align appropriately. However, in the future, we would want to find a way that can make the two graphs align.

Copy link
Member

@huss huss left a comment

Choose a reason for hiding this comment

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

I want to thank @nmqng for taking on this larger issue. I also ask your understanding for how long my review took as my efforts where focused elsewhere.

Overall, this does what the design document requested. I have made a number of comments to consider. In the big picture, I spent time considering how compare line functioned. In the end, based on your efforts, I concluded that changes in the logic were desired. This is due to my having a better understanding of the possibilities and not direct issues with your code. As a result, I have updated the design document with a new updated section to reflect my current thinking. I think the changes may simplify the code. I know you put in a lot of effort to get several of the current ideas implemented and I'm sorry for the changes. I also welcome your thoughts on what I wrote. If you want me to be actively involved in updating the code then please let me know.

src/client/app/translations/data.ts Outdated Show resolved Hide resolved
src/client/app/translations/data.ts Outdated Show resolved Hide resolved
src/client/app/redux/slices/graphSlice.ts Show resolved Hide resolved
src/client/app/components/UIOptionsComponent.tsx Outdated Show resolved Hide resolved
src/client/app/components/CompareLineChartComponent.tsx Outdated Show resolved Hide resolved
src/client/app/components/CompareLineChartComponent.tsx Outdated Show resolved Hide resolved
src/client/app/components/CompareLineChartComponent.tsx Outdated Show resolved Hide resolved
@@ -55,6 +56,14 @@ export interface ThreeDState {
readingInterval: ReadingInterval;
}

export enum ShiftAmount {
week = 'week',
Copy link
Member

Choose a reason for hiding this comment

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

The design document has day as an option. Is there a reason not to add that?

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.

new line comparison graphic
2 participants