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

Tjonx/add color section #10578

Merged
merged 9 commits into from
Sep 19, 2023
Merged

Tjonx/add color section #10578

merged 9 commits into from
Sep 19, 2023

Conversation

tjonx
Copy link
Contributor

@tjonx tjonx commented Sep 19, 2023

WHY are these changes introduced?

Fixes #0000

WHAT is this pull request doing?

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

@tjonx tjonx changed the base branch from main to next September 19, 2023 22:41
@tjonx tjonx marked this pull request as ready for review September 19, 2023 22:41
@tjonx tjonx requested a review from yurm04 September 19, 2023 22:42
Copy link
Contributor

@yurm04 yurm04 left a comment

Choose a reason for hiding this comment

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

I left two comments marked by 🎨 that I'm requesting changes to. The others are not blocking but should be addressed as a fast-follow once we get this up to staging. Looking good other than the few things i pointed out ✨

polaris.shopify.com/src/styles/globals.scss Outdated Show resolved Hide resolved
@@ -202,6 +209,14 @@ body,
--surface-information: #dcf5f0;
--surface-subdued: #f9f9f9;
--surface-warning: #ffeceb;
--fill-success: #29845a;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is fine for now but once this tokens changes make it into next we should swap for polaris tokens

polaris.shopify.com/src/styles/globals.scss Outdated Show resolved Hide resolved
padding: var(--card-padding-lg);
}

.text-extra-padding {
Copy link
Contributor

Choose a reason for hiding this comment

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

🎨 feels weird to have global styles for specific components. This should move to the component as a prop and styling should be applied there or use a layout component

padding: var(--card-padding-md);
}

.directive {
Copy link
Contributor

Choose a reason for hiding this comment

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

Fast follow: let's break this out into it's own component and update any places we're using <Box className="directive"> for the new component before we add anymore documentation

@yurm04 yurm04 merged commit f833921 into next Sep 19, 2023
7 checks passed
@yurm04 yurm04 deleted the tjonx/add-color-section branch September 19, 2023 23:57
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 #0000 <!-- link to issue if one exists -->

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

---------

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants