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

Use the Do/Dont components in /design/colors and remove custom card s… #10597

Merged
merged 4 commits into from
Sep 21, 2023

Conversation

yurm04
Copy link
Contributor

@yurm04 yurm04 commented Sep 20, 2023

WHY are these changes introduced?

Swap out the custom do/dont for the new DoDont components with the updated styling

WHAT is this pull request doing?

Broken
Screenshot 2023-09-20 at 2 23 26 PM

After

Screenshot 2023-09-20 at 2 24 10 PM

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
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>
  );
}

🎩 checklist


const rest = childrenArray.filter((d) => {
if (d.props.src) {
image = d;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pulling out the image from the childrens array

Copy link
Contributor

@gwyneplaine gwyneplaine Sep 21, 2023

Choose a reason for hiding this comment

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

@yurm04 While I appreciate the impetus for this change, automatically making the latest image in children the feature image seems like something that might cause indirection and unintended bugs in the future.

Is there a reason we can't just explicitly passing the image and alt text to the Do / Don't component as props instead (since we're leaning on MDX anyways)


const rest = childrenArray.filter((d) => {
if (d.props.src) {
image = d;
Copy link
Contributor

@gwyneplaine gwyneplaine Sep 21, 2023

Choose a reason for hiding this comment

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

@yurm04 While I appreciate the impetus for this change, automatically making the latest image in children the feature image seems like something that might cause indirection and unintended bugs in the future.

Is there a reason we can't just explicitly passing the image and alt text to the Do / Don't component as props instead (since we're leaning on MDX anyways)

@yurm04
Copy link
Contributor Author

yurm04 commented Sep 21, 2023

@gwyneplaine the old and current DoDonts are authored by nesting markdown (pre-react) and this was a request that came up while writing the gen3 design guidance cc @tjonx. I do personally prefer keeping content writing in markdown as much as possible since I think that's more accessible for the greatest amount of people. Not a strongly held opinion at this point, but i do worry that we're muddying up the authoring experience quite a bit with the amount of react we're introducing in the markdown files

I agree that there is the potential for bugs here. If you're open to keeping this behavior I would totally be down to pair on some more safeguards that can help mitigate the risk for bugs

@tjonx
Copy link
Contributor

tjonx commented Sep 21, 2023

We're going to be much more stricter about how we write directives. ie: No more directives with images within their text. It's always going to either be a text-only directive (no image), or an image followed by the directive. In this case, @yurm04 's solution fits our needs!

I would say sure, it might cause bugs, but that will be intended if folks are not writing directives properly in the future.

And to @yurm04 's point, keeping everything in markdown format makes things easier to implement. It would be weird to switch to props from markdown midway in the markdown file for images, IMO.

@yurm04
Copy link
Contributor Author

yurm04 commented Sep 21, 2023

@gwyneplaine for the sake of time and unblocking content writing we're going to merge in this implementation. I'll create a ticket to re-visit this discussion post v12 launch once we have the time and breathing room to come up with solid opinions informed by @tjonx's comment above

@yurm04 yurm04 merged commit 51a3cf3 into next Sep 21, 2023
7 checks passed
@yurm04 yurm04 deleted the ye/fix-color-cards branch September 21, 2023 14:57
gwyneplaine added a commit that referenced this pull request Oct 11, 2023
### 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.
AnnaCheba pushed a commit to AnnaCheba/polaris that referenced this pull request Apr 22, 2024
Shopify#10597)

<!--
  ☝️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?

Swap out the custom do/dont for the new `DoDont` components with the
updated styling

<!--
  Context about the problem that’s being addressed.
-->

### WHAT is this pull request doing?

Broken
<img width="1624" alt="Screenshot 2023-09-20 at 2 23 26 PM"
src="https://github.com/Shopify/polaris/assets/4642404/64e12bb9-b629-47f2-8ea1-f511d2709750">

After

<img width="1624" alt="Screenshot 2023-09-20 at 2 24 10 PM"
src="https://github.com/Shopify/polaris/assets/4642404/c83fd21e-e539-42bb-92e9-950c5b86927f">


<!--
  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
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.

4 participants