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

docs(chips): Update README to new format #2652

Merged
merged 11 commits into from
Apr 30, 2018
Merged

docs(chips): Update README to new format #2652

merged 11 commits into from
Apr 30, 2018

Conversation

bonniezhou
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented Apr 26, 2018

Codecov Report

Merging #2652 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2652   +/-   ##
=======================================
  Coverage   98.48%   98.48%           
=======================================
  Files          98       98           
  Lines        4230     4230           
  Branches      537      537           
=======================================
  Hits         4166     4166           
  Misses         64       64

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc9ecce...c654f87. Read the comment docs.


## Variants

### Leading and Trailing Icons
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO I'd be inclined to put the different chip types first to quickly follow the example above, rather than require people reading this top-to-bottom to context-switch back later.

Also, do/should we explain anywhere which of leading/trailing are valid on which types of chip?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about that too, but Filter Chips references leading icons...

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... yeah okay.

Should we also mention the use of trailing remove icon under Input Chips... anad maybe also update this documentation since IIRC we determined that remove icon was the only valid use of trailing icon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's better to add the note about input chips under trailing icons and have it linked to the input chips section.
Now it's debatable whether we should do the same for filter chips...but I think filter chips talks about leading icons in a depth that requires more than just a reference.

@@ -153,7 +177,7 @@ CSS Class | Description

### Sass Mixins

To customize the colors of any part of the chip, use the following mixins.
To customize a chip, use the following mixins.
Copy link
Contributor

Choose a reason for hiding this comment

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

We could drop this paragraph, esp. since the first mixin applies to the chip set.

@@ -204,6 +228,10 @@ Property | Value Type | Description
--- | --- | ---
`chips` | Array<`MDCChip`> | An array of the `MDCChip` objects that represent chips in the set

## Usage within Web Frameworks

If you are using a JavaScript framework, such as React or Angular, you can create a <COMPONENT_NAME> for your framework. Depending on your needs, you can use the _Simple Approach: Wrapping MDC Web Vanilla Components_, or the _Advanced Approach: Using Foundations and Adapters_. Please follow the instructions [here](../integrating-into-frameworks.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

<COMPONENT_NAME> -> Chip Set and Chip?

Also, the "here" link needs a different relative path (see #2651)

@@ -81,6 +81,8 @@ You can optionally add a leading icon (i.e. thumbnail) and/or a trailing icon to

#### Trailing icon

A trailing icon comes with the functionality to remove the chip from the set. If you're adding a trailing icon, also set `tabindex="0"` and `role="button"` to make it accessible by keyboard and screenreader. Trailing icons should only be added to [input chips](###input-chips).
Copy link
Contributor

Choose a reason for hiding this comment

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

###input-chips should just be #input-chips, it's linking to another part of the page.

Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

Found a couple more nits from previous content but otherwise LGTM


The MDC Chips module is comprised of two JavaScript classes:
* `MDCChip` defines the behavior of a single chip
* `MDCChip` defines the behavior of a single chip.
Copy link
Contributor

Choose a reason for hiding this comment

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

On the line above this:

  • module -> package (arguably each of those classes is in a JS module)
  • "is comprised of" -> "is composed of" (comprises works the opposite way)

@bonniezhou bonniezhou merged commit 40dd1b8 into master Apr 30, 2018
@bonniezhou bonniezhou deleted the chips/readme branch April 30, 2018 15:08
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.

3 participants