-
Notifications
You must be signed in to change notification settings - Fork 363
Use data attr selectors instead of class selectors #98
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment, otherwise looks awesome.
src/sections/featured-product.liquid
Outdated
@@ -50,7 +50,7 @@ | |||
</label> | |||
|
|||
<select | |||
class="single-option-selector" | |||
data-single-option-selector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we should keep the ordering consistent when inside of a tag:
id/class
- data selector
- data attributes
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can get onboard with that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where would you put stuff like alt
, src
, href
in this list? I wonder if there's a thing in our FED guide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be curious to know if there's anything like this in our FED guide too, will 👀 .
Personal preference but imo any attribute, e.g. href
and src
, that are specific to a tag should come right after the tag itself. For instance:
<img src="smiley.gif" alt="Smiley face" height="42" width="42" class="some-image" data-so-image data-much-wow="doge">
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked into it. There's nothing. I'll just put them at the end.
src/sections/featured-product.liquid
Outdated
@@ -65,7 +65,7 @@ | |||
{% endfor %} | |||
{% endunless %} | |||
|
|||
<select class="no-js product-select" name="id"> | |||
<select data-product-select class="no-js" name="id"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
src/sections/product.liquid
Outdated
@@ -57,7 +57,7 @@ | |||
{% endfor %} | |||
{% endunless %} | |||
|
|||
<select class="no-js product-select" name="id"> | |||
<select data-product-select class="no-js" name="id"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
To address #95