Update design for recommended Test Plans in the support tables #1096
Conversation
April 8th 2024 Production Release
April 29, 2024 Production Release
Paul-Clue
changed the title
There was a problem hiding this comment.
Looks good! Thanks for taking another look at the design supporting the RECOMMENDED copy and color scheme as well!
I've left some additional inline thoughts below.
But the key "issue" is a color change has been missed, when the details panel is open. I tested it on the WAI website to see how it looks and noticed the borders in the expanded view are don't match the green accent. You can also see it on the /embed link by zooming in your browser to the maximum %.
| .details > summary.recommended { | ||
| border: 1.5px solid #7ac498; | ||
| background-color: #e9fbe9; | ||
| } |
There was a problem hiding this comment.
| .details > summary.recommended { | |
| border: 1.5px solid #7ac498; | |
| background-color: #e9fbe9; | |
| } |
Duplicated below rule
| .details > summary > span { | ||
| position: relative; | ||
| padding-left: var(--left-right-padding); | ||
| padding-right: var(--left-right-padding); | ||
| } | ||
|
|
||
| .details > summary > span > h4 { | ||
| font-family: Arial, Helvetica, sans-serif; | ||
| font-size: 1em; | ||
| color: #60470c; | ||
| display: inline-block; | ||
| } |
There was a problem hiding this comment.
It is a bit lost on me why an extra <span> is needed looking at the structure. But I also acknowledge that I'm looking at this after several revisions so perhaps this has already been explained.
Seems to me like the following should give the same result:
.details > summary > h4 {
position: relative;
padding-left: var(--left-right-padding);
padding-right: var(--left-right-padding);
font-family: Arial, Helvetica, sans-serif;
font-size: 1em;
color: #60470c;
display: inline-block;
}| <details id='embed-report-phase-container'> | ||
| <summary id='candidate-title'><span>Warning! Unapproved Report</span></summary> | ||
| <div id='candidate-content-container'> | ||
| <details class="details"> |
There was a problem hiding this comment.
While not wrong to have the class name be the same as the element name, and there may be cases why you'd want to do that, I'm not sure why the need here. To me, it seems better to directly target the details element rather than define a .details class, but this is purely opinion.
| <summary id='candidate-title'><span>Warning! Unapproved Report</span></summary> | ||
| <div id='candidate-content-container'> | ||
| <details class="details"> | ||
| <summary><span><h4>Warning! Unapproved Report</h4></span></summary> |
There was a problem hiding this comment.
| <summary><span><h4>Warning! Unapproved Report</h4></span></summary> | |
| <summary><h4>Warning! Unapproved Report</h4></summary> |
Based on my above comment on not needing to target <span>. Also typically, inline elements shouldn't contain block elements and I can't say I come across much instances of <h*> inside a <span> (but I'm sure there are exceptions!)
| </div> | ||
| </details> | ||
| <details class="details"> | ||
| <summary class="recommended"><span><h4>Recommended Report</h4></span></summary> |
There was a problem hiding this comment.
| <summary class="recommended"><span><h4>Recommended Report</h4></span></summary> | |
| <summary class="recommended"><h4>Recommended Report</h4></summary> |
Same thought as before
…mended-disclosure-in-support-table
…mended-disclosure-in-support-table
| </ol> | ||
| </div> | ||
| </details> | ||
| <details > |
There was a problem hiding this comment.
| <details > | |
| <details> |
…mended-disclosure-in-support-table
…mended-disclosure-in-support-table
|
@Paul-Clue thanks for addressing so much here. Last issue I just came across is that the default
You could fix this by adding the following to the css: details summary::-webkit-details-marker {
display: none;
}Should be good to merge after! |
…mended-disclosure-in-support-table
howard-e
changed the title
see issue #3018
This pull request adds support for tests with a "Recommended" phase in the APG support tables.