Start the tile and basic text part of the app#11
Conversation
This doesn't have much in it, just building the header of the app outlined in the mockup Bug: T263564
| position: relative; | ||
| width: 1024px; | ||
| height: 349px; | ||
| padding: 40px; // $dimension-spacing-xlarge |
There was a problem hiding this comment.
Should we use here the new spacing tokens???
There was a problem hiding this comment.
It's not released yet AFAIK. I couldn't use it directly, that's why I commented it so we can replace it once it's usable.
jakobw
left a comment
jakobw
left a comment
There was a problem hiding this comment.
Looks pretty good! It's great to see the tokens in use in the SCSS :)
Found some small things to nitpick about as usual.
| Short explanatory text that also manages expectations. Short explanatory text that also | ||
| manages expectations. | ||
| </p> | ||
| <h4 class="querybuilder__find-title">Find all items...</h4> |
There was a problem hiding this comment.
This should be an <h2> even if the h4 tokens are used, I think.
There was a problem hiding this comment.
In the figma there's nothing and only it's the css
I'm fine either way, maybe we should double check with the designers?
There was a problem hiding this comment.
I think this should only apply to the styling. As far as semantic markup goes the recommendation is pretty clear:
Avoid skipping heading levels: always start from
<h1>, next use<h2>and so on.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Heading_Elements
There was a problem hiding this comment.
oh thanks then. I fix it.
Ladsgroup
force-pushed
the
basic_text
branch
from
e8eaedd to
b203169
Compare
They went missing after the change.
3 participants
This doesn't have much in it, just building the header of the app
outlined in the mockup
Bug: T263564