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

Question - Table overflow (scrollbars) #7677

Closed
2 of 3 tasks
jmanke opened this issue Sep 5, 2023 · 10 comments
Closed
2 of 3 tasks

Question - Table overflow (scrollbars) #7677

jmanke opened this issue Sep 5, 2023 · 10 comments
Assignees
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Dashboards Issues logged by ArcGIS Dashboards team members. c-table Issues that pertain to the calcite-table and related components calcite-components Issues specific to the @esri/calcite-components package. enhancement Issues tied to a new feature or request. impact - p3 - not time sensitive User set priority impact status of p3 - not time sensitive

Comments

@jmanke
Copy link

jmanke commented Sep 5, 2023

Check existing issues

Description

Currently, I'm having difficulty achieving desired horizontal scrollbar behavior with the calcite-table. In my use case, the user should be able to scroll vertically and horizontally, regardless of where in the table they are. In this example, there is a wrapping div with overflow: auto to allow the user to navigate the table. However, there is an overflow-x: auto within the .table-container inside the shadow root. This is causing a scrollbar to appear at the bottom of the rows, so the user must scroll to the bottom before scrolling left/right - not only that, but now there are two scrollbars. Removing the wrapping div's overflow: x doesn't work either since the user would need to scroll to the bottom of the table before scrolling horizontally. The provided codepen goes into more detail.

Example: codepen

Current double x-scrollbar behavior Desired behavior
Screenshot 2023-09-05 at 10 04 13 AM Screenshot 2023-09-05 at 9 59 38 AM

Question

Is the desired behavior compatible with A11Y, or should I go about this another way? Is this something that will be addressed in #6646?

Acceptance Criteria

Allow user to scroll the calcite-table vertically and horizontally at any point while navigating the table. In my use case, I'm using an outer div to control the overflow. Either thecalcite-table shouldn't implement internal scrollbars (or expose the ability to override it), or the calcite-table should allow the user to scroll vertically/horizontally as seen in the "Desired behavior" screenshot in the description.

Relevant Info

No response

Which Component

calcite-table

Example Use Case

Ability for user to navigate the calcite-table with scrollbars regardless of their current position in the table.

Screenshot 2023-09-05 at 9 59 38 AM

Priority impact

p4 - not time sensitive

Calcite package

  • @esri/calcite-components
  • @esri/calcite-components-react

Esri team

ArcGIS Dashboards

@jmanke jmanke added 0 - new New issues that need assignment. enhancement Issues tied to a new feature or request. needs triage Planning workflow - pending design/dev review. labels Sep 5, 2023
@github-actions github-actions bot added ArcGIS Dashboards Issues logged by ArcGIS Dashboards team members. calcite-components Issues specific to the @esri/calcite-components package. impact - p3 - not time sensitive User set priority impact status of p3 - not time sensitive labels Sep 5, 2023
@driskull
Copy link
Member

driskull commented Sep 5, 2023

@macandcheese seems like the table shouldn't have any scrolling built into it.

@macandcheese
Copy link
Contributor

Scrolling horizontally is something we built in to give some kind of usable experience OOTB at smaller viewports. This is otherwise something end users have to build around this themselves.

It is set inside the component because we display our own UI around the table that needs to remain in place as the table contents scroll - the selection area and slotted actions, and the pagination area:

Screen.Recording.2023-09-05.at.2.02.45.PM.mov

An option could be to expose this value as a public css var (either direct value assignment or an opt-in/out), and let folks opt out of that. I think that would work here.

We could also remove the scrolling altogether when neither of those areas is displayed.

Is the desired behavior compatible with A11Y, or should I go about this another way?

I imagine so but cc @geospatialem to confirm

@driskull
Copy link
Member

driskull commented Sep 5, 2023

I would expect more of a layout component to handle scrolling or let the user handle their own scrollable container.

@macandcheese
Copy link
Contributor

We can offer an opt-out via css variable or other means, but we won't be able to contain the UI around the table like in the above video without the rule.

IMO, we also shouldn't just let the component overflow a container, we should have some guardrails built into the component to prevent that - there are plenty of other components where we sticky content and handle scrolling around it, Panel, Modal, List, etc.

@driskull
Copy link
Member

driskull commented Sep 5, 2023

List doesn't have any scrolling built in from my understanding.

Panel and Modal are both container layout components that accept other components slotted within them. Table only expects table items slotted inside of it. I wouldn't consider table a layout component.

To me, its kind of weird we have scrolling built into a component that doesn't accept anything slotted content inside of it. I would expect to be able to slot a table component into a panel or shell-panel and have scrolling happen there and not have to deal with double scrollbars.

Now that may be different from the advanced table component which may necessarily need scrolling built in depending on its needs and requirements. However, I would expect this component to behave more like a native HTML table and not scroll by default.

@macandcheese
Copy link
Contributor

macandcheese commented Sep 5, 2023

I was referring to the filter stickying within list, because it is similar to us handling scrolling here alongside our internally rendered UI.

Table does accept slotted content, and it does render its own UI alongside the Table element, just not all the time. The selection area and pagination are both rendered inside the component.

Even without that, we would still be providing a component that by default will overflow containers and break layout without a user having to add a container to, most likely, add overflow-x scrolling.

I’m all for exposing those variables via css property or finding another way to make it configurable, we should 100% support that.

@driskull
Copy link
Member

driskull commented Sep 6, 2023

Looking at the panel, it will scroll but only if necessary. When placed in a container and spanning full height it won't scroll. So maybe its just an implementation change for the table to not scroll when taking full width or height.

@macandcheese macandcheese added the c-table Issues that pertain to the calcite-table and related components label Nov 6, 2023
@olga-knyazeva
Copy link

We have a similar issue when having a table inside a modal, there are 2 horizontal scrollbars, in case the minimum content of the table is wider than the viewport. Here the codepen - https://codepen.io/Olga-Knyazeva-the-bashful/pen/yLZweJG (to see the issue, make the viewport width less than 1450px).

@macandcheese macandcheese self-assigned this Dec 14, 2023
@macandcheese macandcheese added 2 - in development Issues that are actively being worked on. and removed 0 - new New issues that need assignment. labels Dec 16, 2023
@geospatialem geospatialem removed the needs triage Planning workflow - pending design/dev review. label Dec 21, 2023
macandcheese added a commit that referenced this issue Dec 21, 2023
**Related Issue:** #7677 

## Summary
Fixes table overflow issues and adds conditional styles to prevent
double borders on last visible table row when pagination is present.

previous (from #7677):
https://codepen.io/jmanke/pen/XWoNNbv?editors=1100
with this pr: https://codepen.io/mac_and_cheese/pen/abXgNdb?editors=1100


previous (from [#7677
comment](#7677 (comment))):
https://codepen.io/Olga-Knyazeva-the-bashful/pen/yLZweJG
with this pr: https://codepen.io/mac_and_cheese/pen/BaMgLLK
@macandcheese macandcheese added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Dec 21, 2023
Copy link
Contributor

Installed and assigned for verification.

@github-actions github-actions bot assigned DitwanP and unassigned macandcheese Dec 21, 2023
@DitwanP
Copy link
Contributor

DitwanP commented Dec 21, 2023

🍡 Verified here

@DitwanP DitwanP closed this as completed Dec 21, 2023
@DitwanP DitwanP added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Dashboards Issues logged by ArcGIS Dashboards team members. c-table Issues that pertain to the calcite-table and related components calcite-components Issues specific to the @esri/calcite-components package. enhancement Issues tied to a new feature or request. impact - p3 - not time sensitive User set priority impact status of p3 - not time sensitive
Projects
None yet
Development

No branches or pull requests

6 participants