Template markup improvement - #138
Merged
Merged
Conversation
Owner
|
Many thanks for the criticism, it definitely deserves an improvement and it's great you've sent the PR! You're right about this being a breaking change so it should definitely target v2.0.0. |
ain
requested changes
Nov 17, 2019
| <div class="smartbanner__info__title">${this.options.title}</div> | ||
| <div class="smartbanner__info__author">${this.options.author}</div> | ||
| <div class="smartbanner__info__price">${this.options.price}${this.priceSuffix}</div> | ||
| <div class="smartbanner__info-container"> |
Owner
There was a problem hiding this comment.
I'd suggest a more laconic copy here as in reference to matter that needs to be printed:
Suggested change
| <div class="smartbanner__info-container"> | |
| <div class="smartbanner__copy"> |
| </div> | ||
| </div> | ||
| <a href="${this.buttonUrl}" target="_blank" class="smartbanner__button" rel="noopener" aria-label="${this.options.button}"><span class="smartbanner__button__label">${this.options.button}</span></a> | ||
| <a href="${this.buttonUrl}" target="_blank" class="smartbanner__button" rel="noopener" aria-label="${this.options.button}"><span class="smartbanner__button-text">${this.options.button}</span></a> |
Owner
There was a problem hiding this comment.
In the legacy context, let's keep it closer to what it was?
Suggested change
| <a href="${this.buttonUrl}" target="_blank" class="smartbanner__button" rel="noopener" aria-label="${this.options.button}"><span class="smartbanner__button-text">${this.options.button}</span></a> | |
| <a href="${this.buttonUrl}" target="_blank" class="smartbanner__button" rel="noopener" aria-label="${this.options.button}"><span class="smartbanner__button-label">${this.options.button}</span></a> |
| &__label { | ||
| text-align: center; | ||
| } | ||
| &__button-text { |
Owner
There was a problem hiding this comment.
Suggested change
| &__button-text { | |
| &__button-label { |
| &__label { | ||
| display: block; | ||
| padding: 0 10px; | ||
| .smartbanner__button-text { |
Owner
There was a problem hiding this comment.
Suggested change
| .smartbanner__button-text { | |
| .smartbanner__button-label { |
| <div class="smartbanner__info__title">Smart Application</div> | ||
| <div class="smartbanner__info__author">SmartBanner Contributors</div> | ||
| <div class="smartbanner__info__price">FREE - On the App Store</div> | ||
| <div class="smartbanner__info-container"> |
Owner
There was a problem hiding this comment.
Suggested change
| <div class="smartbanner__info-container"> | |
| <div class="smartbanner__copy"> |
| </div> | ||
| </div> | ||
| <a href="https://play.google.com/store" target="_blank" class="smartbanner__button" rel="noopener" aria-label="View"><span class="smartbanner__button__label">View</span></a> | ||
| <a href="https://play.google.com/store" target="_blank" class="smartbanner__button" rel="noopener" aria-label="View"><span class="smartbanner__button-text">View</span></a> |
Owner
There was a problem hiding this comment.
Suggested change
| <a href="https://play.google.com/store" target="_blank" class="smartbanner__button" rel="noopener" aria-label="View"><span class="smartbanner__button-text">View</span></a> | |
| <a href="https://play.google.com/store" target="_blank" class="smartbanner__button" rel="noopener" aria-label="View"><span class="smartbanner__button-label">View</span></a> |
| <div class="smartbanner__info__title">Smart Application</div> | ||
| <div class="smartbanner__info__author">SmartBanner Contributors</div> | ||
| <div class="smartbanner__info__price">FREE</div> | ||
| <div class="smartbanner__info-container"> |
Owner
There was a problem hiding this comment.
Suggested change
| <div class="smartbanner__info-container"> | |
| <div class="smartbanner__copy"> |
| </div> | ||
| </div> | ||
| <a href="#" target="_blank" class="smartbanner__button" rel="noopener" aria-label="View"><span class="smartbanner__button__label">View</span></a> | ||
| <a href="#" target="_blank" class="smartbanner__button" rel="noopener" aria-label="View"><span class="smartbanner__button-text">View</span></a> |
Owner
There was a problem hiding this comment.
Suggested change
| <a href="#" target="_blank" class="smartbanner__button" rel="noopener" aria-label="View"><span class="smartbanner__button-text">View</span></a> | |
| <a href="#" target="_blank" class="smartbanner__button" rel="noopener" aria-label="View"><span class="smartbanner__button-label">View</span></a> |
| <div class="smartbanner__info__title">Smart Application</div> | ||
| <div class="smartbanner__info__author">SmartBanner Contributors</div> | ||
| <div class="smartbanner__info__price">FREE - In Google Play</div> | ||
| <div class="smartbanner__info-container"> |
Owner
There was a problem hiding this comment.
Suggested change
| <div class="smartbanner__info-container"> | |
| <div class="smartbanner__copy"> |
| </div> | ||
| </div> | ||
| <a href="https://play.google.com/store" target="_blank" class="smartbanner__button" rel="noopener" aria-label="View"><span class="smartbanner__button__label">View</span></a> | ||
| <a href="https://play.google.com/store" target="_blank" class="smartbanner__button" rel="noopener" aria-label="View"><span class="smartbanner__button-text">View</span></a> |
Owner
There was a problem hiding this comment.
Suggested change
| <a href="https://play.google.com/store" target="_blank" class="smartbanner__button" rel="noopener" aria-label="View"><span class="smartbanner__button-text">View</span></a> | |
| <a href="https://play.google.com/store" target="_blank" class="smartbanner__button" rel="noopener" aria-label="View"><span class="smartbanner__button-label">View</span></a> |
Ge11ert
force-pushed
the
template-markup-improvement
branch
from
November 18, 2019 22:22
1face76 to
b76c626
Compare
Author
|
@ain I dont know how to manage with these "suggested changes", actually :) (It seems, GH offers me to create new commits with applied changes, but I need fixup/amend them) |
1 similar comment
ain
approved these changes
Nov 18, 2019
Owner
|
Many thanks! I'll release the current master as |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hello! I am glad to see you use BEM for class naming convention (at least, it's pertty similar to bem).
But some parts of html template markup contain few mistakes, if we are talking about BEM.
The first (pretty common, but pretty rude) mistake - is having elements of elements. Or having block's grandchilds, if you like.
E.g. in master branch you have a block called
smartbanner, which has an elementsmartbanner__info, which in his turn also has an elementsmartbanner__info__author.Quick link to FAQ with this question.
The second what I noticed - markup has a plain
divtag without any class. Some will judge you for that, some will not.But AFAIK, you should avoid tags without a class, every should have one. Because if you dont have a class, there's no information about which block this element belongs to.
I am absolutely not shure about
smartbanner__info-container, I'm not so inventive, so I would like to accept any suggestion, if you have it.P.S. If these changes will be approved, I think the can go with v-2.0.0, when it comes to light. Because it might be breaking change for those, who rely on classnames.