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.

Sign up for GitHub

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

Jump to bottom

Table width mobile options #4169

Merged
merged 16 commits into from
Oct 27, 2020
Merged

Table width mobile options #4169

merged 16 commits into from
Oct 27, 2020

Conversation

Phizzard
Copy link
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

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
Contributor Author

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

@miukimiu
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

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

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

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 miukimiu October 22, 2020 19:35
cchaos
cchaos suggested changes Oct 22, 2020
View reviewed changes
src-docs/src/views/tables/mobile/mobile.js Outdated Show resolved Hide resolved
src-docs/src/views/tables/mobile/mobile_section.js Outdated Show resolved Hide resolved
src-docs/src/views/tables/mobile/props_info.js Outdated Show resolved Hide resolved
src/components/table/table_row_cell.tsx Outdated Show resolved Hide resolved
src/services/hooks/useIsWithinBreakpoints.ts Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@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
Contributor

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
Contributor Author

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
Contributor Author

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
Contributor

cchaos commented Oct 27, 2020

Jenkins, test this

chandlerprall
chandlerprall approved these changes Oct 27, 2020
View reviewed changes
Copy link
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

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
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

@chandlerprall chandlerprall chandlerprall approved these changes

@cchaos cchaos cchaos approved these changes

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 @miukimiu @chandlerprall @cchaos