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.

Sign up for GitHub

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

Jump to bottom

[FEAT][TABLE][SC-46247] Basic version #223

Merged
merged 12 commits into from
Feb 23, 2024
Merged

[FEAT][TABLE][SC-46247] Basic version #223

merged 12 commits into from
Feb 23, 2024

Conversation

KevinFabre-ods
Copy link
Contributor

@KevinFabre-ods KevinFabre-ods commented Feb 12, 2024 •
edited
Loading

Summary

The goal for this PR is to add a simple version of a table component. This take inspiration from the exploration: #222

(Internal for Opendatasoft only) Associated Shortcut tickets:

Changes

  • Render and style a basic table
  • Add visualization common properties: title, subtitle and source link
  • Add Two Storybook stories, default and a custom theme
  • Make it available as a component in the visualizations/react package

Review checklist

  • Description is complete
  • Commits respect the Conventional Commits Specification
  • 2 reviewers (1 if trivial)
  • Tests coverage has improved
  • Code is ready for a release on NPM

@KevinFabre-ods KevinFabre-ods added the enhancement New feature or request label Feb 12, 2024
@KevinFabre-ods KevinFabre-ods marked this pull request as ready for review February 13, 2024 08:21
@KevinFabre-ods
Copy link
Contributor Author

@etienneburdet To let you know that we've started the development with a basic version

@KevinFabre-ods KevinFabre-ods mentioned this pull request Feb 13, 2024
Closed
etienneburdet
etienneburdet approved these changes Feb 20, 2024
View reviewed changes
Copy link
Contributor

@etienneburdet etienneburdet left a comment

Choose a reason for hiding this comment

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

LGTM functionnally.

Implem wise I'm not a fan of JSON/props based themed, since it makes a lot of discrete options to document, read, pass down as global… I'd rather style directly in CSS. But that's debatable 🤷

packages/visualizations/src/components/Table/utils/theme.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Overall, I'm not a fan of adding props and JS constants for styling. CSS works just as well, it considerably augments the API's surface and in my experience it always ends up with custom styling anyways because we can't hope to cover every case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's true that if we exposed an unstyled prop it is in conflict with the theme prop.
#224

So you are suggesting to remove the theme prop and let developers style the table via CSS directly on table tags (like th, td ...) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would think so, yes. Unless we maybe want to expose a very basic version with 2-5 colors (that would fit the Studio well indeed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The table will takes colors from the domain theme.
I'll set neutral colors as default for the SDK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is something like that you have in mind ?
5266807

packages/visualizations/src/components/Table/Table.svelte Outdated Show resolved Hide resolved
packages/visualizations/src/components/Table/Table.svelte Show resolved Hide resolved
etienneburdet
etienneburdet reviewed Feb 20, 2024
View reviewed changes
Copy link
Contributor

@etienneburdet etienneburdet left a comment

Choose a reason for hiding this comment

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

Ha yes, also the theme persists when switching stories, but I think you fix that in the style PR

etienneburdet

This comment was marked as duplicate.

Sign in to view

@KevinFabre-ods
Copy link
Contributor Author

Ha yes, also the theme persists when switching stories, but I think you fix that in the style PR

Yes, I fixed that with next PRs

@KevinFabre-ods
Copy link
Contributor Author

@etienneburdet I edit a bit the code. Can you check if it's good to start as it is ?

@etienneburdet
Copy link
Contributor

All good for me 🏂

@KevinFabre-ods
chore: add sass language for svelte style
74d1537
@KevinFabre-ods
style(Table): add a unstyled prop
2374486 
Default is false; Is set to true: Remove any presentation style that is applied to the table
@KevinFabre-ods
chore(Table): update table style
f7db9b8 
Use a lot the :global style element to not scope style. HTML don't have a scoped classname, so it's easier to override the default style.
@KevinFabre-ods
feat(Table): format cell content
2b01b46
@KevinFabre-ods
style(Table): Update style for short and long text
c02079e
@KevinFabre-ods
chore(Table): update components organization
2eff5d1
@KevinFabre-ods
style(Table): update scoped classname
756dfcd
@KevinFabre-ods
feat(Table): add locale props
2f6f403 
To format number and dates according to the locale.
Default locale comes from the navigator language.
@KevinFabre-ods KevinFabre-ods merged commit d0bd88d into main Feb 23, 2024
7 checks passed
@KevinFabre-ods KevinFabre-ods deleted the feature/sc-46247/sdk-new-visualization-table branch February 23, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etienneburdet etienneburdet etienneburdet approved these changes

@cybervinvin cybervinvin Awaiting requested review from cybervinvin

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KevinFabre-ods @etienneburdet