The Wayback Machine - https://web.archive.org/web/20210129235811/https://github.com/adobe/react-spectrum/pull/1182
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

Adding support for semantic color for icons #1182

Merged
merged 9 commits into from Oct 24, 2020
Merged

Adding support for semantic color for icons #1182

merged 9 commits into from Oct 24, 2020

Conversation

@LFDanLu
Copy link
Collaborator

@LFDanLu LFDanLu commented Oct 20, 2020

Closes #1159

Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

LFDanLu added 2 commits Oct 20, 2020
LFDanLu added 2 commits Oct 20, 2020
@LFDanLu LFDanLu changed the title (WIP) Adding support for semantic color for icons Adding support for semantic color for icons Oct 20, 2020
@@ -10,6 +10,8 @@
* governing permissions and limitations under the License.
*/

// This file is generated by lib/varsToTypeScript.js! DO NOT EDIT.

This comment has been minimized.

This comment has been minimized.

@LFDanLu

LFDanLu Oct 20, 2020
Author Collaborator

generated by running varsToTypeScript, not entirely sure if we wanna keep or not.

This comment has been minimized.

@snowystinger

snowystinger Oct 20, 2020
Collaborator

but why wasn't it there previously?

This comment has been minimized.

@LFDanLu

LFDanLu Oct 20, 2020
Author Collaborator

dunno, removed in this pull: #206. Not sure the reason for its removal, seems innocuous to me (helps others understand where the file came from)

This comment has been minimized.

@snowystinger

snowystinger Oct 20, 2020
Collaborator

yeah, i'm definitely in favor of leaving it in if it is truly a generated file

```tsx example
import Alert from '@spectrum-icons/workflow/Alert';
<Alert />

This comment has been minimized.

@snowystinger

snowystinger Oct 21, 2020
Collaborator

Should we wrap these in a Flex with a gap so they aren't quite that close together?
Also we should put an aria-label on them, otherwise we'll likely get some warnings

@@ -95,6 +99,8 @@ export type DimensionValue =
| number;

export type ColorValue =
| 'status'
| 'version'

This comment has been minimized.

@devongovett

devongovett Oct 22, 2020
Member

These seem like parsing issues. I don't think they are real variable names?

This comment has been minimized.

@LFDanLu

LFDanLu Oct 23, 2020
Author Collaborator

ah you are right, I'll update the regex in the script

LFDanLu and others added 4 commits Oct 23, 2020
@devongovett devongovett merged commit d6206f7 into main Oct 24, 2020
11 checks passed
11 checks passed
Adobe CLA Signed? ✓ Adobe Employee
Details
ci/circleci: comment Your tests passed on CircleCI!
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: storybook Your tests passed on CircleCI!
Details
ci/circleci: storybook-17 Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: test_17 Your tests passed on CircleCI!
Details
commit Workflow: commit
Details
@devongovett devongovett deleted the issue_1159 branch Oct 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants