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

Data page: About this data (& refactor shared components) #2853

Merged
merged 32 commits into from
Nov 27, 2023

Conversation

sophiamersmann
Copy link
Member

@sophiamersmann sophiamersmann commented Oct 25, 2023

Description

Implements the redesigned About this data section and refactors shared data page components.

About this data section:

  • Updating the design meant adding a few pieces of metadata, including unit, additionalInfo and the indicator's title

Refactor:

  • Component-specific styles are moved into a scss file next to the component file
  • Grid-related styles and classes are removed (shared components simply take up the full width of the parent container)
  • <IndicatorBrief /> is split into two components, <IndicatorKeyData /> and <IndicatorDescriptions /> (makes it a little easier for the sources modal)
  • Metadata helper functions are moved into metadataHelpers.ts in the utils folder

Other changes:

  • I didn't touch any of the logic; with some exceptions (hehe)
    • Origins are deduplicated based on their label (name of producer, title of data product)
    • The heading of a source shows the producer and title of the data product (needs Marwa's input)
  • I fixed minor visual glitches

Open questions

  • Should we keep referring to indicators in code as "variables", or do we want to start using both terms interchangeably?

@sophiamersmann sophiamersmann force-pushed the refactor-datapage-indicator-components branch from 6615f3c to 67c29e3 Compare October 25, 2023 15:08
@sophiamersmann sophiamersmann mentioned this pull request Oct 26, 2023
4 tasks
@sophiamersmann sophiamersmann force-pushed the refactor-grapher-core-column-def branch from 0b22570 to 4022482 Compare October 27, 2023 10:48
@sophiamersmann sophiamersmann force-pushed the refactor-datapage-indicator-components branch from 80bdf07 to ccb1889 Compare October 27, 2023 10:48
@sophiamersmann sophiamersmann marked this pull request as ready for review October 27, 2023 12:07
Base automatically changed from refactor-grapher-core-column-def to master October 27, 2023 15:19
@sophiamersmann sophiamersmann force-pushed the refactor-datapage-indicator-components branch from 9304695 to 1d93632 Compare October 30, 2023 08:53
Copy link
Contributor

@danyx23 danyx23 left a comment

Choose a reason for hiding this comment

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

Very nice work! The components are so much tidier now, love it :)

I have one comment about prefering not to switch to an enum - otherwise this is good to ship!

packages/@ourworldindata/utils/src/OwidVariable.ts Outdated Show resolved Hide resolved
Comment on lines +36 to +38
white-space: nowrap;
text-overflow: ellipsis;
overflow: hidden;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks interesting but exceeds my CSS understanding. What does this do?

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 think this is code I copied over. It makes it so, that the content doesn't wrap into a new line. Instead, if the content overflows, three dots "..." are added at the end of the line

Copy link
Contributor

@danyx23 danyx23 left a comment

Choose a reason for hiding this comment

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

Very nice work! The components are so much tidier now, love it :)

I have one comment about prefering not to switch to an enum - otherwise this is good to ship!

@sophiamersmann
Copy link
Member Author

sophiamersmann commented Oct 30, 2023

@danyx23 Ah, I think for now we're referring to indicators as "variables" in code. Should I rename the components to use "Variable" instead of "Indicator" or should we just start using both terms interchangeably?

@sophiamersmann sophiamersmann force-pushed the refactor-datapage-indicator-components branch 2 times, most recently from 264ba0e to 7aa4e9f Compare October 30, 2023 17:29
Copy link
Contributor

danyx23 commented Oct 30, 2023

Yeah this is a bit tricky. I think when communicating to the user it should always be Indicator. In the database etc it will be Variable for a long time probably. For cases like components it's a bit open. I'd say let's use both and for entirely new components like here use Indicator?

@sophiamersmann
Copy link
Member Author

sounds good to me!

thanks for the quick review, Daniel!

@danyx23 danyx23 force-pushed the data-pages-add-related-research-and-writing branch from f3666b5 to 2943fbe Compare November 24, 2023 16:50
@danyx23 danyx23 force-pushed the refactor-datapage-indicator-components branch from 2623efa to bc6a0b5 Compare November 24, 2023 16:50
Copy link
Contributor

danyx23 commented Nov 27, 2023

Merge activity

  • Nov 27, 6:24 AM: @danyx23 started a stack merge that includes this pull request via Graphite.
  • Nov 27, 6:26 AM: @danyx23 merged this pull request with Graphite.

Base automatically changed from data-pages-add-related-research-and-writing to master November 27, 2023 11:25
@danyx23 danyx23 merged commit d4ab26c into master Nov 27, 2023
10 checks passed
@danyx23 danyx23 deleted the refactor-datapage-indicator-components branch November 27, 2023 11:26
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