Skip to content

Table width mobile options#4169

Merged
cchaos merged 16 commits intoelastic:masterfrom
Phizzard:table-width-mobile-options
Oct 27, 2020
Merged

Table width mobile options#4169
cchaos merged 16 commits intoelastic:masterfrom
Phizzard:table-width-mobile-options

Conversation

@Phizzard
Copy link
Copy Markdown
Contributor

@Phizzard Phizzard commented Oct 22, 2020 •
edited
Loading

Summary

  • Added width to mobileOptions prop for EuiTableRowCell.
  • Created useIsWithinBreakpoints hook.

Fixes #4168

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs
  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Oct 22, 2020

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@Phizzard Phizzard marked this pull request as ready for review October 22, 2020 03:45
@Phizzard
Copy link
Copy Markdown
Contributor Author

Phizzard commented Oct 22, 2020

Would it be okay if this PR could be labelled hacktoberfest-accepted to count towards Hacktoberfest :D

@elizabetdev
Copy link
Copy Markdown
Contributor

elizabetdev commented Oct 22, 2020

jenkins test this

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Oct 22, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_4169/

@cchaos cchaos requested a review from elizabetdev October 22, 2020 16:10
@chandlerprall
Copy link
Copy Markdown
Contributor

chandlerprall commented Oct 22, 2020

Would it be okay if this PR could be labelled hacktoberfest-accepted to count towards Hacktoberfest :D

Looks like that label should be added on merge; from https://hacktoberfest.digitalocean.com/details -

PRs count if:
Submitted during the month of October AND
Submitted in a public repo AND (
  The PR is labelled as hacktoberfest-accepted by a maintainer OR
  Submitted in a repo with the hacktoberfest topic AND (
    The PR is merged OR
    The PR has been approved
  )
)

Once this PR is approved, we'll add that label for ya!

@cchaos cchaos requested review from cchaos and removed request for elizabetdev October 22, 2020 19:35
cchaos
cchaos suggested changes Oct 22, 2020
View reviewed changes
Phizzard and others added 7 commits October 24, 2020 15:14
@Phizzard @cchaos
Update src-docs/src/views/tables/mobile/mobile.js
499d782 
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@Phizzard @cchaos
Update src-docs/src/views/tables/mobile/mobile_section.js
c0b1eb4 
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@Phizzard @cchaos
Update src-docs/src/views/tables/mobile/props_info.js
09ce26d 
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@Phizzard @cchaos
Update src/components/table/table_row_cell.tsx
9e8fe7b 
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@Phizzard @cchaos
Update CHANGELOG.md
f2a4815 
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@Phizzard
add useIsWithinBreakpoints to hooks/index
8989ad9
@Phizzard
fix(table_row_cell): add import for CSSProperties type
620457c
@Phizzard Phizzard requested a review from cchaos October 26, 2020 14:13
chandlerprall
chandlerprall reviewed Oct 26, 2020
View reviewed changes
src/services/hooks/useIsWithinBreakpoints.ts Outdated
* @returns {boolean} Returns `true` if current breakpoint name is included in `sizes`
*/
export function useIsWithinBreakpoints(sizes: EuiBreakpointSize[]) {
const [windowSize, setWindowSize] = useState<boolean>(false);
Copy link
Copy Markdown
Contributor

@chandlerprall chandlerprall Oct 26, 2020

Choose a reason for hiding this comment

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

The names here indicate (at least to me) that it holds the window size, not the status of the hook. Let's rename to isWithinBreakpoints/setIsWithinBreakpoints

Copy link
Copy Markdown
Contributor Author

@Phizzard Phizzard Oct 27, 2020

Choose a reason for hiding this comment

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

I agree - Originally this hook I was building was just tracking window size and setting it before I made it utilize the isWithinBreakpoints Util, little left over from the first iteration

Copy link
Copy Markdown
Contributor Author

@Phizzard Phizzard Oct 27, 2020

Choose a reason for hiding this comment

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

I updated it to [isWithinBreakpointsValue, setIsWithinBreakpointsValue] as isWithinBreakpoints is already taken by the Util function it's using and felt that aliasing it could be just a little confusing enough walking into it to know what's being returned.

@cchaos
Copy link
Copy Markdown
Contributor

cchaos commented Oct 27, 2020

Jenkins, test this

chandlerprall
chandlerprall approved these changes Oct 27, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

pending CI passing, these changes LGTM

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Oct 27, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_4169/

cchaos
cchaos approved these changes Oct 27, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

LGTM too!

@cchaos cchaos merged commit b7de5b3 into elastic:master Oct 27, 2020
@cee-chen cee-chen mentioned this pull request Oct 27, 2020
Merged
3 tasks
@thompsongl thompsongl mentioned this pull request Oct 27, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@chandlerprall chandlerprall chandlerprall approved these changes

@cchaos cchaos cchaos approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[EuiBasicTable] mobileOptions.fullWidth does not override width property

5 participants

@Phizzard @kibanamachine @elizabetdev @chandlerprall @cchaos