Add layout components for improving MDX #10896
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kyledurand
approved these changes
Oct 5, 2023
|
|
||
| interface MdxColumnProps { | ||
| children: React.ReactNode; | ||
| span?: '1/3' | '2/3' | '1/2' | 'full'; |
There was a problem hiding this comment.
nit: I think oneThird, twoThirds, oneHalf, full reads a little better
There was a problem hiding this comment.
Appreciate the feedback! I was following the common naming that Tailwind uses for their percentage width sizing and personally like the fraction notation.
Curious what others think and open to changing it to your suggestion if others agree 👍
There was a problem hiding this comment.
I find 1/3 2/3 1/2 etc. to be superior here. It's faster to understand how many columns / total columns we're working with instead of being verbose.
There was a problem hiding this comment.
I don't have a strong preference. My history with our Layout component is probably making me partial to keep things consistent.
The reason I commented was because of the mix of fractions with full seemed a lil awkward but this is internal only so I'm fine with it
There was a problem hiding this comment.
Hypothesis: I think it is more accessible to a wider audience to have more single purpose components instead of a smaller number of components with APIs that make a single component more flexible.
To explore this hypothesis I opened this draft PR with dedicated components for
TwoColumnsThreeColumns
And maybe some others like
CardTwoColumnsCardThreeColumnsCardGrid
The idea here is that rather than needing to know the API for the component to infer what the page will look like, you can more or less read the markdown/component name and know what it does and how to use it quickly. My assumption is that the increased readability will be easier for non-engineers and folks that are not strong with react/coding can still confidently read and contribute changes with minimum onboarding.
There was a problem hiding this comment.
the increased readability will be easier for non-engineers and folks
+1 to optimizing to readability and easier contribution. I like the TwoColumns and ThreeColumns naming approach–though I am trying to think if this means we leave the Grid approach in favor of a more nested but easier to read and reason-about row/column approach. Should we replace the grid strategy with something more like the following?
<Card>
<ThreeColumn>
<Content></Content>
<Do></Do>
<Dont></Dont>
</ThreeColumn>
</Card>
There was a problem hiding this comment.
I don't think we need to be this verbose though.
<Columns amount=2> makes more sense to me than having separate components for each amount of columns I want. I'm not married to that opinion though (still love the 1/2 1/3 approach the most, it feels efficient).
There was a problem hiding this comment.
With this approach in my head I've been keeping things at max 1 level of nesting, which means that to have a card with a specific grid or column layout we would need each to have a dedicated component.
<CardTwoColumns>
<Do></Do>
<Dont></Dont>
</CardTwoColumns>
<CardGrid>
<Do></Do>
<Dont></Dont>
<DirectiveCard />
<DirectiveCard />
<DirectiveCard />
</CardGrid>
This more verbose naming strategy is fine, as long as we keep the card layout selection to the predictable few that were proposed (I think i was told it'd be 4?). I think this is a strong opinion we should establish early: we should have as few layouts as possible, and make sure that our MDX layout components are added intentionally rather than overwhelm the markdown and contributors
There was a problem hiding this comment.
I'm all for having both!
I love the less-nested, simpler CardTwoColumns for readability and get-started-quicker-ness, and I'm a fan of having a generic Columns for when there's something custom we need to do 👍
There was a problem hiding this comment.
Created an issue so we don't lose this useful discussion: #10980
5 tasks
The Menu button on Safari in dark mode is broken <img width="554" alt="image" src="https://github.com/Shopify/polaris/assets/1266923/01f797d4-3853-4a5c-be6d-83333a666588"> I noticed that we use `filter: invert(1)` on the SVG which is fine if we had any color actually specified on the menu SVG. Without it the menu button can't be inverted and the fact this works on chrome is a bug 🙃 I replaced it with `fill: white`. We can replace `white` with the appropriate token here if there is one in the future but should make sure that the menu button color in light mode is also specified.
### WHY are these changes introduced? Fixes #10937 
<!-- ☝️How to write a good PR title: - Prefix it with [ComponentName] (if applicable), for example: [Button] - Start with a verb, for example: Add, Delete, Improve, Fix… - Give as much context as necessary and as little as possible - Prefix it with [WIP] while it’s a work in progress --> ### WHY are these changes introduced? Fixes #10907 
Bumps [zod](https://github.com/colinhacks/zod) from 3.19.1 to 3.22.3. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/colinhacks/zod/releases">zod's releases</a>.</em></p> <blockquote> <h2>v3.22.3</h2> <h2>Commits:</h2> <ul> <li>1e23990bcdd33d1e81b31e40e77a031fcfd87ce1 Commit</li> <li>9bd3879b482f139fd03d5025813ee66a04195cdd docs: remove obsolete text about readonly types (<a href="https://redirect.github.com/colinhacks/zod/issues/2676">#2676</a>)</li> <li>f59be093ec21430d9f32bbcb628d7e39116adf34 clarify datetime ISO 8601 (<a href="https://redirect.github.com/colinhacks/zod/issues/2673">#2673</a>)</li> <li>64dcc8e2b16febe48fa8e3c82c47c92643e6c9e3 Update sponsors</li> <li>18115a8f128680b4526df58ce96deab7dce93b93 Formatting</li> <li>28c19273658b164c53c149785fa7a8187c428ad4 Update sponsors</li> <li>ad2ee9ccf723c4388158ff6b8669c2a6cdc85643 2718 Updated Custom Schemas documentation example to use type narrowing (<a href="https://redirect.github.com/colinhacks/zod/issues/2778">#2778</a>)</li> <li>ae0f7a2c15e7741ee1b23c03a3bfb9acebd86551 docs: update ref to discriminated-unions docs (<a href="https://redirect.github.com/colinhacks/zod/issues/2485">#2485</a>)</li> <li>2ba00fe2377f4d53947a84b8cdb314a63bbd6dd4 [2609] fix ReDoS vulnerability in email regex (<a href="https://redirect.github.com/colinhacks/zod/issues/2824">#2824</a>)</li> <li>1e61d76cdec05de9271fc0df58798ddf9ce94923 3.22.3</li> </ul> <h2>v3.22.2</h2> <h2>Commits:</h2> <ul> <li>13d9e6bda286cbd4c1b177171273695d8309e5de Fix lint</li> <li>0d49f10b3c25a8e4cbb6534cc0773b195c56d06d docs: add typeschema to ecosystem (<a href="https://redirect.github.com/colinhacks/zod/issues/2626">#2626</a>)</li> <li>8e4af7b56df6f2e3daf0dd825b986f1d963025ce X to Zod: add app.quicktype.io (<a href="https://redirect.github.com/colinhacks/zod/issues/2668">#2668</a>)</li> <li>792b3ef0d41c144cd10641c6966b98dae1222d82 Fix superrefine types</li> </ul> <h2>v3.22.1</h2> <h2>Commits:</h2> <p>Fix handing of <code>this</code> in ZodFunction schemas. The parse logic for function schemas now requires the <code>Reflect</code> API.</p> <pre lang="ts"><code>const methodObject = z.object({ property: z.number(), method: z.function().args(z.string()).returns(z.number()), }); const methodInstance = { property: 3, method: function (s: string) { return s.length + this.property; }, }; const parsed = methodObject.parse(methodInstance); parsed.method("length=8"); // => 11 (8 length + 3 property) </code></pre> <ul> <li>932cc472d2e66430d368a409b8d251909d7d8d21 Initial prototype fix for issue <a href="https://redirect.github.com/colinhacks/zod/issues/2651">#2651</a> (<a href="https://redirect.github.com/colinhacks/zod/issues/2652">#2652</a>)</li> <li>0a055e726ac210ef6efc69aa70cd2491767f6060 3.22.1</li> </ul> <h2>v3.22.0</h2> <h2><code>ZodReadonly</code></h2> <p>This release introduces <code>ZodReadonly</code> and the <code>.readonly()</code> method on <code>ZodType</code>.</p> <!-- raw HTML omitted --> </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/colinhacks/zod/commit/1e61d76cdec05de9271fc0df58798ddf9ce94923"><code>1e61d76</code></a> 3.22.3</li> <li><a href="https://github.com/colinhacks/zod/commit/2ba00fe2377f4d53947a84b8cdb314a63bbd6dd4"><code>2ba00fe</code></a> [2609] fix ReDoS vulnerability in email regex (<a href="https://redirect.github.com/colinhacks/zod/issues/2824">#2824</a>)</li> <li><a href="https://github.com/colinhacks/zod/commit/ae0f7a2c15e7741ee1b23c03a3bfb9acebd86551"><code>ae0f7a2</code></a> docs: update ref to discriminated-unions docs (<a href="https://redirect.github.com/colinhacks/zod/issues/2485">#2485</a>)</li> <li><a href="https://github.com/colinhacks/zod/commit/ad2ee9ccf723c4388158ff6b8669c2a6cdc85643"><code>ad2ee9c</code></a> 2718 Updated Custom Schemas documentation example to use type narrowing (<a href="https://redirect.github.com/colinhacks/zod/issues/2778">#2778</a>)</li> <li><a href="https://github.com/colinhacks/zod/commit/28c19273658b164c53c149785fa7a8187c428ad4"><code>28c1927</code></a> Update sponsors</li> <li><a href="https://github.com/colinhacks/zod/commit/18115a8f128680b4526df58ce96deab7dce93b93"><code>18115a8</code></a> Formatting</li> <li><a href="https://github.com/colinhacks/zod/commit/64dcc8e2b16febe48fa8e3c82c47c92643e6c9e3"><code>64dcc8e</code></a> Update sponsors</li> <li><a href="https://github.com/colinhacks/zod/commit/f59be093ec21430d9f32bbcb628d7e39116adf34"><code>f59be09</code></a> clarify datetime ISO 8601 (<a href="https://redirect.github.com/colinhacks/zod/issues/2673">#2673</a>)</li> <li><a href="https://github.com/colinhacks/zod/commit/9bd3879b482f139fd03d5025813ee66a04195cdd"><code>9bd3879</code></a> docs: remove obsolete text about readonly types (<a href="https://redirect.github.com/colinhacks/zod/issues/2676">#2676</a>)</li> <li><a href="https://github.com/colinhacks/zod/commit/1e23990bcdd33d1e81b31e40e77a031fcfd87ce1"><code>1e23990</code></a> Commit</li> <li>Additional commits viewable in <a href="https://github.com/colinhacks/zod/compare/v3.19.1...v3.22.3">compare view</a></li> </ul> </details> <br /> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/Shopify/polaris/network/alerts). </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
) Add changelog so we can release stylelint breaking changes as a major release We [marked `v14.1.2` as unsafe](https://github.com/Shopify/polaris/releases/tag/%40shopify%2Fstylelint-polaris%4014.1.2) since it had the breaking changes from polaris v12 and polaris-tokens v8
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/stylelint-polaris@15.0.0 ### Major Changes - [#10941](#10941) [`ec1efca1b`](ec1efca) Thanks [@sophschneider](https://github.com/sophschneider)! - - Updated dependencies \[[`2a467249f`](2a46724), [`2cdc59f88`](2cdc59f), [`794d1f5e4`](794d1f5), [`86d4040c05`](86d4040), [`08aaf11ec`](08aaf11), [`a2aefd0`](a2aefd0), [`2ce6c37`](2ce6c37)]: - @shopify/polaris-tokens@8.0.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
### WHY are these changes introduced? The Polaris team recently updated the UI Kits in the Figma community and the links have changed. ### WHAT is this pull request doing? Updates Design resources links to the UI Kit community kits on the Getting started page https://polaris.shopify.com/getting-started#design-resources --------- Co-authored-by: Lo Kim <lo.kim@shopify.com>
* Update migration guide to use safer migration commands from Shopify/app-bridge#3183 * Fix glob paths * Fix some copy mistakes * Delete shadow bevel regex co author: @chloerice commits from [here](https://github.com/Shopify/polaris/compare/update-migration-guide?expand=1) and [here](#10911) --------- Co-authored-by: Sam Rose <11774595+sam-b-rose@users.noreply.github.com> Co-authored-by: Lo Kim <lo.kim@shopify.com> Co-authored-by: Chloe Rice <chloerice@users.noreply.github.com>
<!-- ☝️How to write a good PR title: - Prefix it with [ComponentName] (if applicable), for example: [Button] - Start with a verb, for example: Add, Delete, Improve, Fix… - Give as much context as necessary and as little as possible - Prefix it with [WIP] while it’s a work in progress --> ### WHY are these changes introduced? <img width="1624" alt="Screenshot 2023-10-10 at 2 34 57 PM" src="https://github.com/Shopify/polaris/assets/4642404/8243a2d9-06a9-47f0-ab6a-dcd68f29970b"> <!-- Context about the problem that’s being addressed. --> ### WHAT is this pull request doing? <!-- Summary of the changes committed. Before / after screenshots are appreciated for UI changes. Make sure to include alt text that describes the screenshot. If you include an animated gif showing your change, wrapping it in a details tag is recommended. Gifs usually autoplay, which can cause accessibility issues for people reviewing your PR: <details> <summary>Summary of your gif(s)</summary> <img src="..." alt="Description of what the gif shows"> </details> --> <!-- ℹ️ Delete the following for small / trivial changes --> ### How to 🎩 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) <!-- Give as much information as needed to experiment with the component in the playground. --> <details> <summary>Copy-paste this code in <code>playground/Playground.tsx</code>:</summary> ```jsx import React from 'react'; import {Page} from '../src'; export function Playground() { return ( <Page title="Playground"> {/* Add the code you want to test in here */} </Page> ); } ``` </details> ### 🎩 checklist - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [ ] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [ ] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
### WHY are these changes introduced? Due to the way that DirectiveCard has been [written](#10597), whitespace is necessary in order for the mdx compile to consider the image as a separate node. Otherwise it will wrap the image and the paragraph below it in a single p tag. ### WHAT is this pull request doing? Just adds white space between the image tag and the paragraph below.
The migrator was accidentally added to the changeset ignore list in `main`. It was initially added to the list in `next` to prevent prereleases for the migrator. It should be added back so we can release snapshots of the migrator package.
…#10959) Adds information about the Figma UI Kit updates
Reverts the changes from [here](e34b94e) that were merged into main. We only needed these changes for the `next` branch.
…10950) Updates the Polaris migrator CLI to accept a file list via `stdin`. Example usage with redirects: ```sh npx @shopify/polaris-migrator styles-replace-custom-property \ --decl=color \ --from=--p-text \ --to=--p-color-text \ --stdin < file-list.txt ``` Example usage with piping: ```sh git diff --name-only | npx @shopify/polaris-migrator styles-replace-custom-property \ --decl=color \ --from=--p-text \ --to=--p-color-text \ --stdin ```
sam-b-rose
force-pushed
the
mdx-layout-components
branch
from
22b59cb
to
f9dd3e3
Compare
github-actions
bot
added
the
cla-needed
sam-b-rose
added a commit
that referenced
this pull request
Oct 12, 2023
Same as #10896 but targeting the correct branch `main` instead of `next` 😮💨
AnnaCheba
pushed a commit
to AnnaCheba/polaris
that referenced
this pull request
Apr 22, 2024
Same as Shopify#10896 but targeting the correct branch `main` instead of `next` 😮💨
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #10952
This only affects components used to author MDX content–no visual changes.