Docs DirectiveCard and DoDont refactor
#10581
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
gwyneplaine
force-pushed
the
v12/10571
branch
2 times, most recently
from
ba9aeb4
to
c01e043
Compare
|
Cards look very shoddy in darkmode rn due to hardcoded box-shadow values: Do we have existing shadow tokens (or proposed semantic token / value pair) we can use? If not I'll leave it as something we pick up later |
gwyneplaine
commented
Sep 20, 2023
| > :last-child { | ||
| margin-bottom: 0; | ||
| :last-child { | ||
| margin-block: 0; |
There was a problem hiding this comment.
Removing the > operator so that even when wrapped inStack the last child node will still have its block margin removed.
Not sure if this is a wise choice or not, so happy for an alternative suggestion.
gwyneplaine
commented
Sep 20, 2023
| import ImageThumbnail from '../../../ThumbnailPreview'; | ||
| import styles from './styles.module.scss'; | ||
|
|
||
| export enum DirectiveStatusName { |
There was a problem hiding this comment.
Not sure how I feel about ts enum as an API, but using it here for consistency since we also use this pattern in StatusBadge.
gwyneplaine
commented
Sep 20, 2023
| ); | ||
| }; | ||
|
|
||
| export const Pill = ({status}: {status: DirectiveStatus}) => { |
There was a problem hiding this comment.
Decided to just reimplement StatusBadge here, mainly because I don't want to tie these Directive status' to the status in StatusBadge canonically used to denote component and page status (new deprecated etc.)
Not a strongly held opinion, happy to refactor if others have strong opinions 👍
gwyneplaine
requested review from
yurm04 and
jesstelford
and removed request for
yurm04
… a composition of the DirectiveCard
gwyneplaine
force-pushed
the
v12/10571
branch
from
c01e043
to
c4fe9a2
Compare
yurm04
approved these changes
Sep 20, 2023
| @@ -44,8 +43,7 @@ export const DoDont = ({children}: PropsWithChildren) => { | |||
| while (i < childrenArray.length) { | |||
| target = childrenArray[i].props.children.startsWith('Don') ? donts : dos; | |||
|
|
|||
| // Add the heading | |||
| target.push(childrenArray[i]); | |||
| // skip the headings in older uses of <DoDont> | |||
There was a problem hiding this comment.
removed the heading since it looks redundant with the new design
Before
After
yurm04
reviewed
Sep 20, 2023
| @@ -1,5 +1,9 @@ | |||
| @import '../../styles/mixins.scss'; | |||
| .Card { | |||
| @include dark-mode { | |||
| box-shadow: var(--card-shadow); | |||
There was a problem hiding this comment.
added to match the other "cards" in darkmode around the site
5 tasks
AnnaCheba
pushed a commit
to AnnaCheba/polaris
that referenced
this pull request
Apr 22, 2024
<!-- ☝️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 Shopify#10571 ### WHAT is this pull request doing? - `Docs` Add DirectiveCard component, and refactor DoDont components as a composition of the DirectiveCard - `Docs` update Card styles to override last-child margin of *all* children With Image: <img width="450" alt="Screenshot 2023-09-20 at 4 21 31 pm" src="https://github.com/Shopify/polaris/assets/12119389/dd05c87c-7e99-4eee-85da-16c84b80e931"> Do Don't: <img width="894" alt="Screenshot 2023-09-20 at 4 22 04 pm" src="https://github.com/Shopify/polaris/assets/12119389/b6a53b61-f6f1-4315-ab67-2e08e75f6884"> ### 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 --------- Co-authored-by: Yuraima Estevez <yuraima.estevez@shopify.com>
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.
WHY are these changes introduced?
Fixes #10571
WHAT is this pull request doing?
DocsAdd DirectiveCard component, and refactor DoDont components as a composition of the DirectiveCardDocsupdate Card styles to override last-child margin of all childrenWith Image:
Do Don't:
How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx:🎩 checklist
README.mdwith documentation changes