Skip to content

Conversation

devonpmack
Copy link
Contributor

@devonpmack devonpmack commented Jan 22, 2020

WHY are these changes introduced?

Needed in Web because we are using the DataTable in a modal and the scroll indicator doesn't look very good.

image

WHAT is this pull request doing?

Adding a prop showNavigationOnCollapse (default true), that when false, hides the scroll indicator.

image

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';
import {Card, DataTable, Page} from '@shopify/polaris';

export function Playground() {
  return (
    <Page title="Sales by product">
      <Card sectioned>
        <Card.Section>
          <DataTable
            columnContentTypes={[Array(30).fill('text')]}
            headings={Array(30).fill('Header')}
            rows={[Array(30).fill('Content')]}
            hideScrollIndicator
          />
        </Card.Section>

        <Card.Section>
          <DataTable
            columnContentTypes={[Array(30).fill('text')]}
            headings={Array(30).fill('Header')}
            rows={[Array(30).fill('Content')]}
          />
        </Card.Section>
      </Card>
    </Page>
  );
}

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@github-actions
Copy link
Contributor

github-actions bot commented Jan 22, 2020

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

No significant changes to src/**/*.tsx were detected.


This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

@dleroux
Copy link
Contributor

dleroux commented Jan 22, 2020

@chloerice since you have TONES of context on the DataTable, do you see an issue with doing this?

Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

Thanks for contributing @devonpmack, allowing the table navigation to be optional totally makes sense. Suggested a couple edits to more clearly indicate that the navigation isn't toggled on and off by the prop alone, since the navigation is only visible when the table collapses to fit its container. Naming is hard though 😅so pinging @sadiesaurus for extra input 🙏

@chloerice chloerice requested a review from sadiesaurus January 22, 2020 19:01
@devonpmack
Copy link
Contributor Author

Thanks for your insight @chloerice!

Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Code looks good to me. The prop name caught me off-guard at first though. I'm wondering if hideScrollIndicator would be a better name and default to false.

I'm not held to this opinion but I feel like showNavigationOnCollapse sounds like it forces the navigation to show when the data table is collapsed which is the default behaviour anyway.

@chloerice
Copy link
Member

Code looks good to me. The prop name caught me off-guard at first though. I'm wondering if hideScrollIndicator would be a better name and default to false.

I'm not held to this opinion but I feel like showNavigationOnCollapse sounds like it forces the navigation to show when the data table is collapsed which is the default behaviour anyway.

Hmm, yeah I think your right @kyledurand. hideScrollIndicator better indicates that showing the navigation is the default.

@PabloVallejo
Copy link
Contributor

I like the idea of hideScrollIndicator too. It's a bit easier to read.

@devonpmack devonpmack force-pushed the datatable/scroll-indication-prop branch from 5c70e67 to f95d049 Compare January 22, 2020 21:08
@devonpmack
Copy link
Contributor Author

devonpmack commented Jan 22, 2020

Changed to hideScrollIndicator and default to false :)

@devonpmack devonpmack requested a review from chloerice January 22, 2020 21:09
@devonpmack devonpmack force-pushed the datatable/scroll-indication-prop branch from 8ea2b61 to 00af471 Compare January 22, 2020 21:10
@devonpmack devonpmack force-pushed the datatable/scroll-indication-prop branch from 00af471 to f3fa88a Compare January 22, 2020 21:11
@devonpmack devonpmack requested a review from dleroux January 22, 2020 21:23
@devonpmack devonpmack merged commit 49e48ec into master Jan 22, 2020
@devonpmack devonpmack deleted the datatable/scroll-indication-prop branch January 22, 2020 22:29
@dleroux dleroux temporarily deployed to production January 27, 2020 13:38 Inactive
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.

6 participants