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

Polish Markdown component #1477

Merged
merged 40 commits into from
Feb 20, 2023
Merged

Polish Markdown component #1477

merged 40 commits into from
Feb 20, 2023

Conversation

bharatkashyap
Copy link
Member

@bharatkashyap bharatkashyap commented Dec 20, 2022 •
edited by oliviertassinari
Loading

  • Closes Polish markdown component #1470
  • Visual demonstration
  • Replace react-markdown with markdown-to-jsx
  • Uses Material UI Link instead of a
  • Links open in new tabs through target=_blank
  • No margin
Screen.Recording.2022-12-20.at.4.07.58.PM.mov

@bharatkashyap bharatkashyap added design: ux Design enhancement This is not a bug, nor a new feature labels Dec 20, 2022
@oliviertassinari oliviertassinari requested a deployment to polish-markdown - toolpad-db PR #1477 December 20, 2022 10:39 — with Render Abandoned
@bharatkashyap bharatkashyap changed the title Review: Markdown Polish Markdown component Dec 20, 2022
@oliviertassinari oliviertassinari temporarily deployed to polish-markdown - toolpad PR #1477 December 22, 2022 06:04 — with Render Destroyed
@oliviertassinari oliviertassinari temporarily deployed to polish-markdown - toolpad PR #1477 December 22, 2022 06:04 — with Render Destroyed
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 24, 2022
@oliviertassinari oliviertassinari requested a deployment to polish-markdown - toolpad-db PR #1477 January 3, 2023 10:19 — with Render Abandoned
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 3, 2023
@oliviertassinari oliviertassinari temporarily deployed to polish-markdown - toolpad PR #1477 January 3, 2023 10:19 — with Render Destroyed
@bharatkashyap
Copy link
Member Author

bharatkashyap commented Jan 3, 2023 •
edited
Loading

Addressing review feedback:

We need the markdown component to have a min-height when it's empty so that developers can make changes.

and

We might want to handle <code><pre> and <code>

Screenshot 2023-01-03 at 3 49 47 PM

Also adding a toolpad-markdown prefix to ids to get around

To see if the ids can break toolpad

Screenshot 2022-12-20 at 11 44 57

Screenshot 2023-01-03 at 3 58 07 PM

Janpot
Janpot reviewed Jan 4, 2023
View reviewed changes
packages/toolpad-components/src/Text.tsx Outdated
component: CodeContainer,
},
},
slugify: (text) => `toolpad-markdown-${text}`,
Copy link
Member

@Janpot Janpot Jan 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

But this would turn the links into #toolpad-markdown-my-id. When is id a problem exactly?
Perhaps we should just remove the ids for now?

Copy link
Member

Choose a reason for hiding this comment

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

When is id a problem exactly?

No idea. I left the feedback in case we rely on getElementById. If we don't, I think that keeping the id is simpler.

Copy link
Member

Choose a reason for hiding this comment

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

In some cases, yes, for the overlay container. Next.js relies on it for a few things, and the MUI X datagrid as well. And I think GA as well. There are also a bunch of autogenerated ones both in Toolpad and MUI core, mostly for a11y reasons I think.

@Janpot
Copy link
Member

Janpot commented Jan 4, 2023

markdown-to-jsx is very popular, but who is it losing market share to?

Screenshot 2023-01-04 at 18 06 05

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 16, 2023
@bharatkashyap bharatkashyap requested review from a team and removed request for Janpot February 16, 2023 08:29
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 16, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 16, 2023
Janpot
Janpot reviewed Feb 16, 2023
View reviewed changes
packages/toolpad-components/src/Text.tsx Outdated
h5: { ...theme.typography.h5, ...gutters(4, 4) },
h6: { ...theme.typography.h6, ...gutters(4, 4) },
p: { margin: 0, marginBottom: 6 },
maxWidth: 'fill-available',
Copy link
Member

@Janpot Janpot Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
maxWidth: 'fill-available',

This doesn't seem to do anything for me. Which situation is this solving?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to introduce scrollbars in content that spans the width of the container:

Without fill-available:
Screenshot 2023-02-16 at 5 19 12 PM

With:
Screenshot 2023-02-16 at 5 19 29 PM

Copy link
Member

Choose a reason for hiding this comment

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

Oh, ok, I see. Does it have advantages over maxWidth: '100%'?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing specifically I guess, I can use 100% as well

@oliviertassinari oliviertassinari temporarily deployed to polish-markdown - toolpad PR #1477 February 17, 2023 11:31 — with Render Destroyed
@oliviertassinari oliviertassinari temporarily deployed to polish-markdown - toolpad PR #1477 February 17, 2023 11:53 — with Render Destroyed
Janpot
Janpot requested changes Feb 17, 2023
View reviewed changes
packages/toolpad-components/src/Text.tsx Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari temporarily deployed to polish-markdown - toolpad PR #1477 February 17, 2023 11:58 — with Render Destroyed
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 19, 2023
@oliviertassinari oliviertassinari requested a deployment to polish-markdown - toolpad-db PR #1477 February 20, 2023 12:51 — with Render Abandoned
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 20, 2023
@bharatkashyap bharatkashyap merged commit 9678892 into master Feb 20, 2023
@bharatkashyap bharatkashyap deleted the polish-markdown branch February 20, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari left review comments

@Janpot Janpot Janpot approved these changes

Assignees
No one assigned
Labels
design: ux Design enhancement This is not a bug, nor a new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Polish markdown component
3 participants
@bharatkashyap @Janpot @oliviertassinari