Fixes issue 575: change h1 to h4 styled as h1 #587
Conversation
src/views/empty-state.html
Outdated
| @@ -2,13 +2,13 @@ | |||
| <div ng-if="$ctrl.config.icon" class="blank-slate-pf-icon"> | |||
| <span class="{{$ctrl.config.icon}}"></span> | |||
| </div> | |||
| <h1 id="title"> | |||
| <h4 id="empty-state-title-{{$id}}" class="h1 title"> | |||
There was a problem hiding this comment.
Hi, it's a little confusing having a <h4> with a class of 'h1 title'?
There was a problem hiding this comment.
h1 is just the bootstrap way to style a different heading as an h1, thats the only reason i kept it. I don't know that we need to keep the class .title either, i just kept it so that the test updates are changes from #title to .title. Def open to any suggestions, was just going for the most minimal changes to make this work!
There was a problem hiding this comment.
Can we avoid using title as a class and just use this new id as a selector in the tests?
There was a problem hiding this comment.
Works for me, I'll update.
src/views/empty-state.html
Outdated
| @@ -2,13 +2,13 @@ | |||
| <div ng-if="$ctrl.config.icon" class="blank-slate-pf-icon"> | |||
| <span class="{{$ctrl.config.icon}}"></span> | |||
| </div> | |||
| <h1 id="title"> | |||
| <h4 id="empty-state-title-{{$id}}" class="h1 title"> | |||
There was a problem hiding this comment.
Can we avoid using title as a class and just use this new id as a selector in the tests?
src/views/empty-state.html
Outdated
| </h1> | ||
| <p id="info" ng-if="$ctrl.config.info"> | ||
| </h4> | ||
| <p id="empty-state-info-{{$id}}" class="info" ng-if="$ctrl.config.info"> |
There was a problem hiding this comment.
Can we use this new ID in the tests rather than adding info as a class?
There was a problem hiding this comment.
I can update this in the tests, though want to bring up that someone targeting with css will have a lousy time trying to get at these without a class hook at all (and such a specific id as empty-state-info-54. Are we sure we don't want a hook at all? (I tend to come from the camp that believes you should never write css targeting dom nodes).
There was a problem hiding this comment.
OK, I can buy into that. How about using pf-empty-state-xxx for the classes though?
src/views/empty-state.html
Outdated
| {{$ctrl.config.info}} | ||
| </p> | ||
| <p id="helpLink" ng-if="$ctrl.config.helpLink"> | ||
| <p id="empty-state-helpLink-{{$id}}" class="helpLink" ng-if="$ctrl.config.helpLink"> |
There was a problem hiding this comment.
Same reply as above. Just to confirm, I'm ok with the change if y'all are certain something like empty-state-title & empty-state-helplink isn't helpful for those who may need to tweak styles (though I'd prob shorten classes and go for .empty-state .helplink by preference, but I'll update to whatever is current convention).
|
Also, we are using semantic versioning now. Please see the Commit Guidelines. |
|
Awesome, I'll update to:
If that works. Is there a convention for scope in |
|
the just becomes the highlighted word for the fix/enhancement. We should use the component name being updated/added as a convention, so pfEmptyState in this case. |
…id's generated by view - fix issue patternfly#575 - h1 changed to h4 styled as h1 - update id's to be unique using {{$id}} - example: id="blank-state-pf-title-{{$id}}"
benjaminapetersen
force-pushed
the
bpeterse/issue/575/empty-state-h1
branch
from
c47d1ed to
cd163eb
Compare
|
Updated, though I matched classes/ids to |
h1toh4.h1id="empty-state-title-{{$id}}"@jeff-phillips-18 for review, might be some conventions I should conform to.
@spadgett
Re: update needed for origin-web-console PR 1326 via this comment.