Skip to content
This repository was archived by the owner on Jan 23, 2024. It is now read-only.

Conversation

TylerAPfledderer
Copy link
Collaborator

@TylerAPfledderer TylerAPfledderer commented Mar 29, 2022

📝 Description

This pull request surveys live code previews site-wide and wraps the code in the TableContainer component so the previews do not overflow the container when viewing in mobile screens.

This does not address any issues with the examples themselves not rendering as intended in mobile screens.

Found sections to fix:

⛳️ Current behavior (updates)

One or more live preview examples in the docs overflow their react-live preview container when viewed on a mobile screen

🚀 New behavior

The wrapping TableContainer component with render an overflow-x: auto to the offending examples to create a horizontal scroll.

💣 Is this a breaking change (Yes/No):

No

📝 Additional Information

Still in draft as all doc pages have not been reviewed. Description of the pull request may change based on further findings.

Using TableContainer instead applying a global overflow: auto style to the react-live preview component was recommended by the docsite maintainer.

Consider this PR to be an interim temporary solution to at least some of the examples in question that need to be redone. (In that case, open separate issues/PRs for each example)

… with `TableContainer` to prevent overflow in mobile screens
@vercel
Copy link

vercel bot commented Mar 29, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/chakra-ui/chakra-ui-docs/5Lm3GyYhPKYD9ToNprtLEZGD3b6j
✅ Preview: https://chakra-ui-docs-git-fork-tylerapfledderer-fix-l-322937-chakra-ui.vercel.app

@TylerAPfledderer TylerAPfledderer changed the title docs: wrap examples with overflow issues with the TableContainer component docs: wrap examples with overflow issues in the TableContainer component Mar 30, 2022
…or checkbox group" with TableContainer to prevent overflow in mobile screens
…input" with TableContainer to prevent overflow in mobile screens
…bleContainer to prevent overflow in mobile screens
Copy link
Collaborator

@noobinthisgame noobinthisgame left a comment

Choose a reason for hiding this comment

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

This looks good to me! I would only put the comment for Tablecontainer in my one place. But that's personal preference I guess.

@TylerAPfledderer
Copy link
Collaborator Author

TylerAPfledderer commented Mar 30, 2022

This looks good to me! I would only put the comment for Tablecontainer in my one place. But that's personal preference I guess.

@noobinthisgame yea I thought about that.

But if anyone added a new example that had the potential of overflow, I would think to also add a PR to the Contributing file and docsite page to add instructions to add the TableContainer component and the visual comment.

@nikolovlazar
Copy link
Contributor

I thought about this and I think we need to be smarter about it. I know it fixes the issue, but here are some cons that I have thought of:

  • All examples are now "tainted" with the TableContainer, plus the comment explaining why it's there. With that we're also damaging the "copy/paste" experience, because the users will need to clean it up after pasting the example in their project.
  • Some beginners could get confused because they're seeing a component named TableContainer in the Tooltip docs for example.
  • If we have a problem with the responsiveness, the users should not be affected by the solution. This solution is more of a compromise.

Why don't we just modify the CodeBlock component in src/components/mdx-components/codeblock/codeblock.tsx? That way all of the examples will have the responsiveness fix that the table container offers, but it won't be part of the examples and won't be visible to the users.

@TylerAPfledderer
Copy link
Collaborator Author

@nikolovlazar agreed! Probably better to close this PR and open a fresh one?

@TylerAPfledderer
Copy link
Collaborator Author

TylerAPfledderer commented Mar 31, 2022

Also, @nikolovlazar, just to clarify: wouldn't adding overflow prop to the LiveCodePreview baseStyles also work?

Edit: I just played around with it. The overflow should be applied to the LiveCodePreview in the baseStyles. Currently there is not a direct way to apply the overflow from the codeblock.tsx file, as the preview container in this file is being rendered via:

if (isMounted && language === 'jsx' && _live === true) {
  return <ReactLiveBlock editable {...reactLiveBlockProps} />
}

Also, with the example from the section Sample Usage for a Radio or Checkbox Group, say, the text content at the end of a row of elements would become compressed, and may need to be resolved with something like whiteSpace: nowrap or pre on the preview container to retain their length compared to how it's viewed on desktop.

@noobinthisgame
Copy link
Collaborator

I'm sorry guys I mixed this PR up with another one. Your guys proposal is what we want!

@TylerAPfledderer
Copy link
Collaborator Author

TylerAPfledderer commented Mar 31, 2022

All good, @noobinthisgame! It was still good to show the TableContainer in those Table Doc examples for best practice. 😄

@nikolovlazar
Copy link
Contributor

Yeah I think so too @TylerAPfledderer. The example you shared does have a longer text and on mobile it goes out of the preview box. The ReactLiveBlock component is actually a wrapper and it exists in the project here:

src/components/mdx-components/codeblock/react-live-block.tsx

The preview is rendered in the <LiveCodePreview zIndex='1' />. You can wrap it with a box and apply the style props you think it'll solve the overflow issue.

@TylerAPfledderer
Copy link
Collaborator Author

@nikolovlazar so wrapping <LiveCodePreview zIndex='1' /> with a box like this:

<Box overflowX='auto' whiteSpace='pre'>
  <LiveCodePreview zIndex='1' />
</Box>

Shows this:
image

I believe applying the changes to line 10 in react-live-block.tsx will solve this issue since they will be applied within LiveCodePreview

const LiveCodePreview = chakra(LivePreview, {
  baseStyle: {
    fontFamily: 'body',
    mt: 5,
    p: 3,
    borderWidth: 1,
    borderRadius: '12px',

    overflowX: 'auto',
    whiteSpace: 'pre'
  },
})

image

@nikolovlazar
Copy link
Contributor

@TylerAPfledderer that's the solution! Apply those changes, clean up the PR and let me know when it's ready to be reviewed.

@TylerAPfledderer
Copy link
Collaborator Author

Closing this pull request to avoid polluting the commit history with bad commits. Opening a new PR to push the proper changes for the same overflow issue.

@TylerAPfledderer TylerAPfledderer deleted the fix/live-examples-overflow branch April 3, 2022 14:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants