Skip to content

Conversation

@brendanrygus
Copy link
Contributor

WHY are these changes introduced?

Issue

Context

WHAT is this pull request doing?

Restores missing borders for the non-scrolling view of the IndexTable cells, by using border instead of box-shadow.

Before, missing borders:
Screen Shot 2021-06-28 at 11 45 16 AM

After, borders restored:
Screen Shot 2021-06-28 at 11 46 02 AM

This does not fix the more complex behaviour where box shadows were used for scrolling sticky cells.
I did some quick testing and it doesn't look like we can swap in borders 1:1 for box-shadows here. I don't think I have the bandwidth to explore a full solution, but if anyone wants to pair on it, let me know.

Chrome:
Screen Shot 2021-06-28 at 11 46 02 AM

Safari:
Screen Shot 2021-06-28 at 11 52 56 AM

How to 🎩

  • Open Google Chrome v91+
  • yarn storybook and navigate to any IndexTable example

🎩 checklist

@brendanrygus brendanrygus requested a review from a team June 28, 2021 16:06
@brendanrygus brendanrygus self-assigned this Jun 28, 2021
@ghost
Copy link

ghost commented Jun 28, 2021

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 28, 2021

size-limit report

Path Size
cjs 142.05 KB (0%)
esm 95.68 KB (0%)
esnext 138.96 KB (+0.01% 🔺)
css 33.82 KB (+0.01% 🔺)

@brendanrygus brendanrygus force-pushed the 4247-index-table-border branch from d0fc7ba to 0dfc0b2 Compare June 28, 2021 17:28
@brendanrygus brendanrygus requested a review from cdaviduik June 28, 2021 18:41
@pamelahicks
Copy link
Contributor

Hi @brendanrygus 👋 what about the right border/box-shadow that you can see separating those first sticky table cells in your Safari screenshot? Those seem to be missing in Chrome as well. Is that something you could add as part of this PR?

@brendanrygus
Copy link
Contributor Author

Hi @brendanrygus 👋 what about the right border/box-shadow that you can see separating those first sticky table cells in your Safari screenshot? Those seem to be missing in Chrome as well. Is that something you could add as part of this PR?

I did explicitly mention this in the PR body:

This does not fix the more complex behaviour where box shadows were used for scrolling sticky cells.
I did some quick testing and it doesn't look like we can swap in borders 1:1 for box-shadows here. I don't think I have the bandwidth to explore a full solution, but if anyone wants to pair on it, let me know.

I think the partial solution is better than none, given how prominent these tables are in our apps. I would not be able to investigate the rest until sometime next week at the earliest, but am open to other contributions on this branch.

@pamelahicks
Copy link
Contributor

pamelahicks commented Jun 28, 2021

I think the partial solution is better than none, given how prominent these tables are in our apps. I would not be able to investigate the rest until sometime next week at the earliest, but am open to other contributions on this branch.

Sounds good, I have a solution for this that I will add to the branch I'm working on.

@brendanrygus
Copy link
Contributor Author

Closing in favour of the more comprehensive solution in #4150
Thanks @pamelahicks!

@alex-page alex-page deleted the 4247-index-table-border branch March 22, 2022 04:18
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.

Recently, I found that the indextable component of Polaris has problems when displaying, how can I solve it?

4 participants