New design #12
New design #12
Conversation
|
Looks pretty. |
|
Generally looks good. Few remarks: this text is not very readable. it's too thin and has little contrast
same here
the badges look bad, because the text is not perfectly centered. also the font size looks a bit too big for the size of the badge
See bootstrap for comparison:
https://getbootstrap.com/docs/5.1/components/badge/#background-colors |
styles/index.less
Outdated
| & > table.styled-table { | ||
| & > thead { font-size: 130%; } | ||
| & > tbody { font-size: 100%; } | ||
|
|
||
| & > thead > tr > th { background: inherit; } | ||
| & > tbody > tr.stale { opacity: 60%; } | ||
| } |
There was a problem hiding this comment.
| & > table.styled-table { | |
| & > thead { font-size: 130%; } | |
| & > tbody { font-size: 100%; } | |
| & > thead > tr > th { background: inherit; } | |
| & > tbody > tr.stale { opacity: 60%; } | |
| } | |
| & > table.styled-table { | |
| display: block; | |
| overflow-x: auto; | |
| & > thead { font-size: 130%; } | |
| & > tbody { font-size: 100%; } | |
| & > thead > tr > th { background: inherit; } | |
| & > tbody > tr.stale { opacity: 60%; } | |
| } |
The site breaks a bit on mobile.
This would make the table vertically scrollable instead.
There was a problem hiding this comment.
See the other suggestion; it's already implemented via a helper class.
|
I liked the "Build problem" badges in red. Grey means "feel free to ignore" to me. Otherwise agree with @davidak |
| <noscript> | ||
| <p class="jsfallback">This page requires Javascript.</p> | ||
| </noscript> | ||
|
|
||
| <table class="table table-striped"> | ||
| <table class="styled-table"> |
There was a problem hiding this comment.
diff --git a/index.html b/index.html
index 8f0a044..7e12ad8 100644
--- a/index.html
+++ b/index.html
@@ -63,18 +63,20 @@
<p class="jsfallback">This page requires Javascript.</p>
</noscript>
- <table class="styled-table">
- <thead>
- <tr>
- <th>Channel</th>
- <th>Last updated</th>
- <th>Commit</th>
- <th>Hydra job for tests</th>
- <th>Channel Status</th>
- </tr>
- </thead>
- <tbody id="channel-status"></tbody>
- </table>
+ <div class="responsivescroll-table">
+ <table class="styled-table">
+ <thead>
+ <tr>
+ <th>Channel</th>
+ <th>Last updated</th>
+ <th>Commit</th>
+ <th>Hydra job for tests</th>
+ <th>Channel Status</th>
+ </tr>
+ </thead>
+ <tbody id="channel-status"></tbody>
+ </table>
+ </div>
</div>
</main>(Can't suggest as it goes outside the diff span)
This will make only the table content scrollable.
It is okay to use the class names, or alternatively the mixins could be used.
| } | ||
|
|
||
| .label { | ||
| font-size: 100%; |
There was a problem hiding this comment.
This is breaking the labels, and this is why they're not vertically aligned anymore. To feel correctly aligned, manual nudging needs to be done, padding-top: 0.4em; seems to fix it
More testing needs to be done, but this seems universal enough?
padding-top: calc(0.1rem + 0.3em);
It would need to be applied to the common styles.
styles/index.less
Outdated
| #container(); | ||
|
|
||
| & > table.styled-table { | ||
| & > thead { font-size: 130%; } |
There was a problem hiding this comment.
I wouldn't change the default for one specific site.
| & > thead { font-size: 130%; } |
Changing the font size all over isn't great. Are there specific concerns this is about?
There was a problem hiding this comment.
This was about getting the table more readable.
| & > thead { font-size: 130%; } | ||
| & > tbody { font-size: 100%; } | ||
|
|
||
| & > thead > tr > th { background: inherit; } |
There was a problem hiding this comment.
Why are we straying from the design here?
| & > thead > tr > th { background: inherit; } |
I mean this as a real question. If there are issues or concern, maybe it should be addressed a the common styles instead of in every projects.
There was a problem hiding this comment.
Probably there is a style consern we should look at since table in its default form was not readable enough (to small).
| & > header { | ||
| &:extend(.navigation-bar all); | ||
|
|
||
| margin-bottom: 0; | ||
| } | ||
|
|
||
| // TODO: why is this not possible? | ||
| //& > div.page-title { | ||
| // &:extend(.page-title all); | ||
| //} |
There was a problem hiding this comment.
I think I see the problem.
The assumed structure for a page was:
body
header
...
main
.page-title
...
This can be fixed by instead relying on
header + div.page-title {
margin-top: -1rem;
} Though this has to be fixed at the common styles repo to be more proper.
| & > header { | |
| &:extend(.navigation-bar all); | |
| margin-bottom: 0; | |
| } | |
| // TODO: why is this not possible? | |
| //& > div.page-title { | |
| // &:extend(.page-title all); | |
| //} | |
| & > header { | |
| &:extend(.navigation-bar all); | |
| } |
|
A reminder that I had a sample of the DOM here: There are a few differences compared to my draft (used to design the table component) I don't mind if it's not used, I just wanted to make sure you realized I had prepared things out. Also note that the icons are in common styles. |
Co-authored-by: Samuel Dionne-Riel <[email protected]>
Co-authored-by: Samuel Dionne-Riel <[email protected]>
Co-authored-by: Samuel Dionne-Riel <[email protected]>
Co-authored-by: Samuel Dionne-Riel <[email protected]>
Co-authored-by: Samuel Dionne-Riel <[email protected]>
@samueldr Can you look at the
styles/index.lessif everything is implemented ok?