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

feat(component): allow state override of table select all checkbox #224

Merged
merged 1 commit into from
Oct 24, 2019

Conversation

deini
Copy link
Member

@deini deini commented Oct 24, 2019

  • allow state override of table select all checkbox
  • itemName prop nos on root level

@deini deini requested a review from a team October 24, 2019 17:57
const { selectAllState, selectedItems } = selectable;

switch (selectAllState) {
case 'ALL':
Copy link
Member

Choose a reason for hiding this comment

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

🍹 consider putting these strings in a constants definition or similar

@@ -70,7 +111,8 @@ export const Actions = memo(
padding="small"
Copy link
Contributor

Choose a reason for hiding this comment

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

🍹Could you try and remove the style={{ marginLeft: 'auto' }} on the pagination, and add justifyContent="space-between" to StyledActions? We should try and clean up those style attributes.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, we have multiple actions, one of them being the itemName so if we do space-between we won't be getting what we want :(

However, I can move this to a custom-styled Flex.

We should add auto to MarginProps

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh yeah, well in that case maybe you could change the Flex.Item for both SelectAll and ItemName to have alignSelf="flex-start" and Pagination alignSelf="flex-end"? 🤷‍♀️

@deini deini merged commit b64eda1 into bigcommerce:master Oct 24, 2019
@deini deini deleted the tableItemName branch October 24, 2019 18:30
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.

3 participants