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

Add is-last-column and is-slack classes to table cells #876

Merged
merged 5 commits into from
Mar 19, 2021

Conversation

ahmacleod
Copy link
Contributor

@ahmacleod ahmacleod commented Mar 2, 2021

Changes:

  • Adds an is-slack class to slack th and td elements
  • Adds an is-last-column class to the rightmost column cells. In slack modes, the rightmost column can be either the slack column (when slack is visible) or its immediate neighbor to the left (when no slack is visible).
  • Adds tests for the above and the previously-untested is-first-column class
  • Improves the page object to better handle slack and nested columns

@ahmacleod ahmacleod marked this pull request as draft March 2, 2021 16:39
@ahmacleod ahmacleod force-pushed the alex.macleod.temp/indicate-last-column branch 2 times, most recently from e769542 to 86dccd7 Compare March 3, 2021 21:33
@ahmacleod ahmacleod marked this pull request as ready for review March 3, 2021 21:49
@ahmacleod ahmacleod marked this pull request as draft March 3, 2021 23:18
@ahmacleod ahmacleod force-pushed the alex.macleod.temp/indicate-last-column branch from 86dccd7 to 3ed58ca Compare March 3, 2021 23:27
@ahmacleod ahmacleod force-pushed the alex.macleod.temp/indicate-last-column branch from 3ed58ca to 492202c Compare March 3, 2021 23:35
@ahmacleod ahmacleod marked this pull request as ready for review March 15, 2021 21:11
@ahmacleod ahmacleod requested review from a team and jiayingxu March 15, 2021 22:03
Copy link
Contributor

@twokul twokul left a comment

Choose a reason for hiding this comment

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

Looks good. Left a few notes ✍️

*/
contentHeaders: collection('th:not([data-test-ember-table-slack])', Header),
slackHeaders: collection('th[data-test-ember-table-slack]', Header),
Copy link
Contributor

Choose a reason for hiding this comment

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

I am assuming this private API. Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. Moved "offline" for a broader discussion.

In this case, contentHeaders is redundant, unused internally, and new since 3.0. It's probably safe to assume nobody is using it. More clarity would be good here though.

@ahmacleod ahmacleod merged commit c26eeec into master Mar 19, 2021
@ahmacleod ahmacleod deleted the alex.macleod.temp/indicate-last-column branch March 19, 2021 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants