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

[List] Fix w3c issues #10050

Merged
merged 2 commits into from
Jan 27, 2018
Merged

[List] Fix w3c issues #10050

merged 2 commits into from
Jan 27, 2018

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jan 26, 2018

I have fixed a lot of errors πŸŽ‰ :

  • [Dividers]:
    Error: Element β€œhr” not allowed as child of element β€œul” in this context.
    Reason: hr is used as a direct chid of ul (I am not 100% sure if this is in component source or demo/docs only)
  • [Lists]:
    Error: Bad value β€œbutton” for attribute β€œrole” on element β€œli”
    Reason: May be demo error only.

It's tricky to get this right. it's related to #9867

@oliviertassinari oliviertassinari added bug πŸ› Something doesn't work component: list This is the name of the generic UI component, not the React module! labels Jan 26, 2018
@oliviertassinari oliviertassinari self-assigned this Jan 26, 2018
@oliviertassinari oliviertassinari force-pushed the w3c-list branch 2 times, most recently from 1b64909 to 6570fde Compare January 27, 2018 00:13
@mbrookes mbrookes self-requested a review January 27, 2018 00:15
@oliviertassinari oliviertassinari removed their assignment Jan 27, 2018
Copy link
Member

@mbrookes mbrookes left a comment

Choose a reason for hiding this comment

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

Great work! Just a few minor nit-picks.

@@ -8,8 +8,14 @@ components: Divider

## List Dividers

The divider renders as a `<hr>` by default.
You can save this DOM element by using the `divider` property on the `ListItem` component.
Copy link
Member

Choose a reason for hiding this comment

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

"save rendering this DOM element"

Copy link
Member

Choose a reason for hiding this comment

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

You could perhaps add an example of that to the List demos (or revise an existing one), and link to it from here.

{{"demo": "pages/demos/dividers/ListDividers.js"}}

## Inset Dividers

The following example demonstrates the `inset` property.
We need to make sure the `Divider` is rendered as a `li` to match the HTML5 specification.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps add: "The example shows two ways of achieving this."

// Don't automatically add the role="button" property on these components.
// It's invalid HTML syntax.
const INVALID_COMPONENT_ROLE = ['a'];

/**
* `ButtonBase` contains as few styles as possible.
* It aims to be a building block for people who want to create a simple button.
Copy link
Member

Choose a reason for hiding this comment

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

"It aims to be a building block for creating a simple button."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug πŸ› Something doesn't work component: list This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants