mercadolibre / chico

A collection of easy-to-use UI components.
http://chico.mercadolibre.com
MIT License
342 stars 91 forks source link

Misuse of attribute `type` in pagination's links #1213

Closed battaglr closed 8 years ago

battaglr commented 10 years ago

To define previous and next links, we are using:

.ch-pagination a[type='prev'],
.ch-pagination a[type='next'] {
    /* ... */
}

.ch-pagination a[type='prev']:hover,
.ch-pagination a[type='next']:hover {
    /* ... */
}

But the type attribute should be used to define the media type for the link target. If we want to express relationship between the link and the target the attribute to use would be rel.

Anyhow, for styling purposes, I suggest using a class, say ch-pagination-sequence, and use the rel attribute to add semantic meaning. This would result in a simpler and faster selector, while reducing the amount of CSS needed.

.ch-pagination .ch-pagination-sequence {
    /* ... */
}

.ch-pagination .ch-pagination-sequence:hover {
    /* ... */
}

What do you think? I would be glad to PR if you agree in the change.

atma commented 9 years ago

I agree that there is a wrong use of an attribute type and should be replaced by rel.

When the prev and next links are using only symbols, e.g. « and », aria-label="Previous" and aria-label="Next" should be used.

When the current page is 1 the prev button should look as disabled. unavailable classname is suggested instead of disabled for semantic reasons, since our element is A and cannot be disabled. The element is unavailable for user at this moment or at this state but still exist and can be accessed. Also recommended to make this links non-reactive by adding the style .ch-pagination .unavailable { pointer-events: none }. The same for the next link.

I'm not sure about additional classes, .ch-pagination .ch-pagination-sequence has more specificity than .ch-pagination a or even .ch-pagination > li > a.

@hmammana @battaglr are invited to conversation.

battaglr commented 9 years ago

I agree that there is a wrong use of an attribute type and should be replaced by rel.

Great! We should update the examples in the docs then.

When the prev and next links are using only symbols, e.g. « and », aria-label="Previous" and aria-label="Next" should be used.

I agree with this. As far as I know, we are not using symbols but words in current implementations, e.g. "Next" in the "demo page", "Siguiente" in Argentina and "Próxima" in Brazil.

I think that the aria-label could be described in the docs —and we need a more extensive one—, since we also need to consider its content localization —if we don't do that, I'm sure that most people will leave it in English, regardless the language in which it's being implemented, and that could be worse that no aria-label at all.

Bottom line: I would open this as a separated issue to discuss it further if you find it necessary.

When the current page is 1 the prev button should look as disabled. unavailable classname is suggested instead of disabled for semantic reasons, since our element is A and cannot be disabled. The element is unavailable for user at this moment or at this state but still exist and can be accessed. Also recommended to make this links non-reactive by adding the style .ch-pagination .unavailable { pointer-events: none }. The same for the next link.

I agree that the class unavailable is the best one to describe the use-case you are presenting. That being said, I think that the best alternative to that use-case it's to avoid including links that are "unavailable" at all —and that's exactly what we are doing, for example, in Argentina and Brazil. Why would we give to the user the possibility to perform an action that he will never be able to perform? Notice that you are comparing this use-case to a situation where "disabled" is a possible state, and that does not really apply here. A use-case for a "disabled" state could be a button that is disabled until the user fills-in some information, and then, its state turns to "enabled". All those state changes occur on the same "page", which is not the case for our "pagination" component.

Bottom line: IMO I would not add the unavailable state class, and discourage any use of the disabled state as well.

I'm not sure about additional classes, .ch-pagination .ch-pagination-sequence has more specificity than .ch-pagination a or even .ch-pagination > li > a.

Our current implementation includes:

.ch-pagination {}

.ch-pagination li {}

.ch-pagination li a {}

.ch-pagination .ch-pagination-current a {}

.ch-pagination .ch-pagination-current a:hover {}

.ch-pagination li a:hover {}

.ch-pagination a[type="prev"]:hover,
.ch-pagination a[type="next"]:hover {}

.ch-pagination a[type="prev"],
.ch-pagination a[type="next"] {}

To overcome the specificity of .ch-pagination li a and .ch-pagination li a:hover we will need to declare this as .ch-pagination .ch-pagination-sequence a and .ch-pagination .ch-pagination-sequence a:hover respectively —just as we do with .ch-pagination .ch-pagination-current a and .ch-pagination .ch-pagination-current a:hover.

An implementation of this could be seen here.

We could also put the .ch-pagination li a:hover declaration just after .ch-pagination li a to achieve better readability.

Bottom line: I would be happy to do a PR adding those changes if you agree.