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

[sc-46254] Add CSS style for vertical scroll #236

Closed
wants to merge 3 commits into from
Closed

[sc-46254] Add CSS style for vertical scroll #236

wants to merge 3 commits into from
+43 −5

Conversation

cybervinvin
Copy link
Contributor

Summary

The goal for this PR is to add a fixed header when the Table is scrollable vertically.

(Internal for Opendatasoft only) Associated Shortcut ticket: sc-46254.

vscroll.mov

Changes

  • add verticalScroll property
  • add new CSS entry header-background-color
  • add new CSS entry vertical-scroll-height
  • thead is sticky on top
  • new story for Vertical Scroll

Breaking Changes

None

Changelog

In the Table component, the header is now sticky on top when the table is scrollable vertically.

Open discussion

When the verticalScroll property is set to true, table height is set arbitrarily and the background color too (default: #ffffff). It's not ideal but I don't see an alternative.

To be tested

A new story has been created for storybook.

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

@cybervinvin cybervinvin added the enhancement New feature or request label May 13, 2024
@cybervinvin cybervinvin force-pushed the feature/sc-46254/sdk-table-fixed-headers-and-footers branch from 4788c15 to 996855b Compare May 13, 2024 06:13
@cybervinvin cybervinvin marked this pull request as ready for review May 13, 2024 06:14
@cybervinvin cybervinvin force-pushed the feature/sc-46254/sdk-table-fixed-headers-and-footers branch from 996855b to 719bb66 Compare May 13, 2024 13:01
etienneburdet
etienneburdet requested changes May 20, 2024
View reviewed changes
packages/visualizations/src/components/Table/Table.svelte Outdated

.vscroll-wrapper {
display: inline-block;
height: var(--vertical-scroll-height);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather have that as 100% and have the container in the app fix the height. Case is, in the Studio we often define height by ratio.

This implies adding an height: 100%; flex-wrap: no wrap;  on the Card.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

packages/visualizations/src/components/utils/Card.svelte
@@ -47,6 +47,8 @@
--spacing-75: 9px;
--spacing-100: 13px;
--border-color: #cbd2db;
--header-background-color: #ffffff;
--vertical-scroll-height: 20em;
/* FIXME: Only using flex style to center source link */
display: flex;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could a use a flex-wrap: nowrap. It's a direct consequence of this PR, but it becomes more obvious now. Else we can end up in horizontal layout in some cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

packages/visualizations-react/stories/Table/Table.stories.tsx Outdated
@@ -28,6 +28,12 @@ Playground.args = {
options,
};

export const VerticalScroll = Template.bind({});
Copy link
Contributor

Choose a reason for hiding this comment

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

So if we go for the 100% on the Table, we'll need to add a container with a fixed height (and maybe one with an aspect-ratio?) to match our use cases.

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 container was added to the story but I did not create a new story withaspect-ratio (I did not know the property)

etienneburdet
etienneburdet requested changes May 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.

Just one thing about height, that I would prefer to have set by the outer app, notably for our aspect ratio use case.

Else I'm fine with all the implem: sticky, conditional class, props to header 👌

@KevinFabre-ods
Copy link
Contributor

Just one thing about height, that I would prefer to have set by the outer app, notably for our aspect ratio use case.

Else I'm fine with all the implem: sticky, conditional class, props to header 👌

I agree with Etienne; developers using our library need to be able to set the height of the table.

You can see that major UI libraries expose a prop to control the max height of the table body:

We just need to find what fits us.

@cybervinvin cybervinvin force-pushed the feature/sc-46254/sdk-table-fixed-headers-and-footers branch from b3b4ca0 to 0612f11 Compare May 24, 2024 07:05
@KevinFabre-ods
Copy link
Contributor

@cybervinvin with the latest merges, we now have conflicts in this PR (so sorry 🥹 )

No rush to fix it. Please stay focus on your dev (loading state) and your reviews

@cybervinvin
Copy link
Contributor Author

No problem, I'll fix it 😉

@cybervinvin
feat(Table): header's table is sticky for scroll
af3728f 
More details:
- add `verticalScroll` property
- add new CSS entry `header-background-color`
- add new CSS entry `vertical-scroll-height`
- `thead` is sticky on top
- new story for `Vertical Scroll`
@cybervinvin
fix(Table) full height for component
38b1df8 
After code review, `Card` should have a height of 100% to allow better control from the application.
@cybervinvin
fix(Table) lint
a465256
@cybervinvin cybervinvin force-pushed the feature/sc-46254/sdk-table-fixed-headers-and-footers branch from 0612f11 to a465256 Compare May 31, 2024 06:49
@KevinFabre-ods
Copy link
Contributor

We will re-open it when we are asked to support it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etienneburdet etienneburdet Awaiting requested review from etienneburdet

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.

None yet
3 participants
@cybervinvin @KevinFabre-ods @etienneburdet