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

[EuiMarkdownEditor] Better support to extend to full height #4245

Conversation

elizabetdev
Copy link
Contributor

@elizabetdev elizabetdev commented Nov 10, 2020

Summary

Closes #4241.

This PR adds a "full" option to the height prop of the EuiMarkdownEditor to better support extend it to full height.

It also adds a maxHeight prop. This way consumers have more options to control the height.

The autoExpandPreview is very common for markdown editors like this one from GitHub. If the content is very long the height expands to fit all the content rather than adding a scrollbar. I decided to provide this functionality as a prop but enable it by default.

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

@elizabetdev elizabetdev marked this pull request as ready for review November 10, 2020 15:14
@kibanamachine
Copy link

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

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.

It would be great to also have an example to shows how to grow the markdown editor to fill a container's height.

src/components/markdown_editor/markdown_editor.tsx Outdated Show resolved Hide resolved
src/components/markdown_editor/markdown_editor.tsx Outdated Show resolved Hide resolved
src/components/markdown_editor/markdown_editor.tsx Outdated Show resolved Hide resolved
@elizabetdev elizabetdev marked this pull request as draft November 12, 2020 10:30
@kibanamachine
Copy link

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

@elizabetdev
Copy link
Contributor Author

Long story short

I tried to add textareaProps and previewProps so we could override the height/styles and try to make the editor/preview full height. But... I found this way a little bit complex. I could have added a good example of how to expand the EuiMarkdownEditor to full height using these props to override, but I decided to try the magical way.

The magical way

So the magical way is for the prop height you can pass a number or pass"full" to fill the height of its container. It seems to be working but requires more testing.

Preview should get the size of the textarea when it's resized

Something that I noticed is that when we resize the textarea and we change the mode to preview, the height of the preview is the initial height.

Screen Recording 2020-11-12 at 06 22 PM

I tried to fix this and it almost works. But the code is not working the way I would like.

Screen Recording 2020-11-12 at 06 03 PM

I also noticed that in GitHub Markdown Editor if we don't have enough content and we resize the textarea the preview gets the same height. Buf if the preview has a lot of content it grows in height to show all the content (no vertical scrollbar).

Is this something we should also allow or get by default? I think it's a good UX for previewing the content. Let me know your thoughts.

@kibanamachine
Copy link

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

@elizabetdev elizabetdev marked this pull request as ready for review November 18, 2020 18:14
@elizabetdev elizabetdev requested a review from cchaos November 18, 2020 18:15
@kibanamachine
Copy link

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

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.

The functionality works great and feels intuitive! 👍

I think it would be work noting that the heights and max-heights don't take the toolbar into account. Meaning the toolbar adds an extra ~40px to the height they pass in.

src/components/markdown_editor/_variables.scss Outdated Show resolved Hide resolved
src/components/markdown_editor/markdown_editor.test.tsx Outdated Show resolved Hide resolved
src/components/markdown_editor/markdown_editor.tsx Outdated Show resolved Hide resolved
src/components/markdown_editor/markdown_editor.tsx Outdated Show resolved Hide resolved
src/components/markdown_editor/markdown_editor.tsx Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

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.

Nice!! Couple small asks but everything looks really well put together. In the end, everything you said probably has a better way does but not without unrelated effort to enable those better ways - and the workarounds look right to me.

I pushed one change to detecting the preview's height to fully respond to any border & margin.

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@elizabetdev
Copy link
Contributor Author

@chandlerprall I went through your comments and pushed the suggested changes.

But I noticed that when the markdown editor has errors, on resizing, the height doesn't update as expected. If I resize the editor in edit mode and go to preview mode and then back again to edit mode, the size goes back to the initial one.

This issue only happens when there are errors. Could it be something related to the component reloading and going back to the initial state? I tried to find the issue and no luck.

Screen Recording 2020-12-14 at 12 33 PM

@chandlerprall
Copy link
Contributor

But I noticed that when the markdown editor has errors, on resizing, the height doesn't update as expected. If I resize the editor in edit mode and go to preview mode and then back again to edit mode, the size goes back to the initial one.

This issue only happens when there are errors. Could it be something related to the component reloading and going back to the initial state? I tried to find the issue and no luck.

I'll check it out!

@kibanamachine
Copy link

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

@chandlerprall
Copy link
Contributor

But I noticed that when the markdown editor has errors, on resizing, the height doesn't update as expected. If I resize the editor in edit mode and go to preview mode and then back again to edit mode, the size goes back to the initial one.

The footer height was read when the component mounted, and the errors only render after the initial mount & parse. The error button adjusts the footer height by a few pixels, so the recorded height didn't match the new height, which triggered a size reset.

I've swapped height tracking to respond to any footer resize instead of only on mount, and couldn't get it into a bad state - though I'd love some extra testing.

@kibanamachine
Copy link

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

@elizabetdev
Copy link
Contributor Author

Thanks @chandlerprall. 🙌🏽

I tested again in Safari, Chrome, Edge, and Firefox, and the issue is fixed.

@elizabetdev elizabetdev requested a review from cchaos January 6, 2021 15:46
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.

Looks and works beautifully! Really nice job. Docs are really clear too.

I only found a couple things that could also be hand-waved or addressed later and just has to do with the responsiveness.

  1. The auto-height of the preview style doesn't update it's height on window resize, only when you toggle between code & preview. So you'll get scrollbars.

Screen Recording 2021-01-06 at 11 29 15 AM

  1. At some point we should figure out a better responsive layout for the toolbar.

Screen Shot 2021-01-06 at 11 19 40 AM


Other than a few other code suggestions. 👍 LGTM!!

@kibanamachine
Copy link

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

@elizabetdev
Copy link
Contributor Author

Thanks @cchaos. I added the snippets as you suggested. I'll make sure I'll address later points 1 and 2.

@elizabetdev elizabetdev merged commit b0395e5 into elastic:master Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiMarkdownEditor] Should be able to fill container height (work in flex context)
4 participants