Skip to content

Conversation

@stkao05
Copy link
Contributor

@stkao05 stkao05 commented Nov 1, 2023

Closes #

Changelog

Changed

Currently, the MarkdownEditor's fullHeight prop doesn't take effect because the height change isn't applied to the outermost container element (<fieldset>). The PR fixes the issue by applying full height style to the the container element.

Storybook has also been updated for demo purpose.

(Demo screen recording: after the fix, the MarkdownEditor correctly resizes to full height when fullHeight is turned on)

Screen.Recording.2023-11-01.at.4.51.37.PM.mov

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan

Testing & Reviewing

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Storybook)
  • Changes are SSR compatible
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

@changeset-bot
Copy link

changeset-bot bot commented Nov 1, 2023

🦋 Changeset detected

Latest commit: b3ead6c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@stkao05 stkao05 force-pushed the markdown-editor-fullheight branch from 5243680 to 1a97aea Compare November 1, 2023 09:01
@stkao05 stkao05 temporarily deployed to github-pages November 1, 2023 09:06 — with GitHub Actions Inactive
@stkao05 stkao05 force-pushed the markdown-editor-fullheight branch from 1a97aea to e425e2f Compare November 1, 2023 09:09
@stkao05 stkao05 temporarily deployed to github-pages November 1, 2023 09:13 — with GitHub Actions Inactive
@stkao05 stkao05 force-pushed the markdown-editor-fullheight branch from e425e2f to 3a64db0 Compare November 1, 2023 15:15
@stkao05 stkao05 marked this pull request as ready for review November 1, 2023 15:16
@stkao05 stkao05 requested review from a team and siddharthkp November 1, 2023 15:16
@@ -0,0 +1,5 @@
---
Copy link
Member

Choose a reason for hiding this comment

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

There are 2 changesets here, I imagine you only want the patch one?

<MarkdownEditor.Label visuallyHidden={hideLabel}>Markdown Editor Example</MarkdownEditor.Label>
</MarkdownEditor>
<p>Note: for demo purposes, files starting with &quot;A&quot; will be rejected.</p>
<div style={{height: '400px'}}>
Copy link
Member

Choose a reason for hiding this comment

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

🤔 what's the purpose of this height?

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 set the height of this element so we can test if the child MarkdownEditor component does extends to full height to its parent element height.

But I agree it is a bit confusing from the code. I can add comment or remove the change if you think it isn't needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove the change for now.

Copy link
Member

@siddharthkp siddharthkp left a comment

Choose a reason for hiding this comment

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

Looking good, left a couple of comments

@stkao05 stkao05 force-pushed the markdown-editor-fullheight branch 2 times, most recently from c90be57 to a268f5b Compare November 7, 2023 13:17
@stkao05 stkao05 requested a review from siddharthkp November 7, 2023 13:19
@stkao05 stkao05 force-pushed the markdown-editor-fullheight branch from a268f5b to ed1daaf Compare November 7, 2023 13:20
@stkao05
Copy link
Contributor Author

stkao05 commented Nov 7, 2023

Thanks for reviewing. Just update the PR accordingly.

@siddharthkp
Copy link
Member

siddharthkp commented Nov 7, 2023

@stkao05, There are some recent changes to MarkdownEditor in main as well, can you merge latest main into your branch?

@stkao05 stkao05 force-pushed the markdown-editor-fullheight branch from bab7615 to ed1daaf Compare November 7, 2023 17:53
@stkao05
Copy link
Contributor Author

stkao05 commented Nov 7, 2023

There are some recent changes to MarkdownEditor in main as well, can you merge latest main into your branch?

no problem. just did 👌

Copy link
Member

@siddharthkp siddharthkp 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! Will merge after CI passes :)

@siddharthkp siddharthkp added this pull request to the merge queue Nov 8, 2023
Merged via the queue into primer:main with commit a4006e2 Nov 8, 2023
@primer primer bot mentioned this pull request Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants