-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Title bars #956
Title bars #956
Conversation
This comment has been minimized.
This comment has been minimized.
Note: If #982 is closed before the review of this PR, we must apply here the Title Bars to the documentation templates. Otherwise, we need to modify the description of the issue to notify the developer to use Title Bars. |
3cd1807
to
7a468c9
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First series of comments without digging at the examples and .scss.
<div class="container"> | ||
<h1>Svg image</h1> | ||
<svg aria-hidden="true" focusable="false"> | ||
<use xlink:href="/docs/{{< param docs_version >}}/assets/img/boosted-sprite.svg#logo"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It is not a good practice to have the logo here. Replace it with a simple inline SVG and remove it from site/static/docs/5.1/assets/img/boosted-sprite.svg.
- It is said further that the title should not wrap: it is the case here.
Moreover, the SVG shouldn't be right-aligned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, it wraps because of the width of the main container, do you think that I should add : word-break: keep-all
?
@louismaximepiton #1009 is merged so we should try to include Title bars into "Examples" and "Guidelines" pages for the titles. |
aa909b8
to
3c5fffc
Compare
1d6245b
to
a590126
Compare
9dba6f2
to
f66e091
Compare
I have completed a design review of the title bars and there are no issues - great work! |
Thanks @Franco-Riccitelli for your review and comments. Considering the rules you mentioned, we added them to complete the documentation part accordingly. By the way, it would be useful to have theses rules also mentioned in ODS. |
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
…xample page Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
36e09f9
to
bbfba51
Compare
Added a CSS section in the documentation via d02f9f0 |
|
||
border-bottom: var(--#{$boosted-prefix}title-bar-border-width) solid var(--#{$boosted-prefix}title-bar-border-color); | ||
|
||
@include media-breakpoint-up(md) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently in the DSM we have to following sizes for the header: 60px 50px 30px. It looks like .display-1
. Can't we use this class instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I understand, the breakpoints are different between the DSM for this component and our breakpoints in Boosted regarding headers... Let's add .display-1
in this PR and fix the behavior in the CSS, and then create an issue to align the DSM and Boosted for the headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a reminder for the one that will create the issue, the actual breakpoints for display-*
are sm
and lg
. Here, we need md
and xl
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final review with 2 small comments. After the fixes, let's merge 🚀
Signed-off-by: Louis-Maxime Piton <louismaxime.piton@orange.com> Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com> Signed-off-by: Julien Déramond <isabelle.chanclou@orange.com> Co-authored-by: Louis-Maxime Piton <louismaxime.piton@orange.com> Co-authored-by: Isabelle Chanclou <isabelle.chanclou@orange.com> Co-authored-by: Julien Déramond <julien.deramond@orange.com>
Previews :
https://deploy-preview-956--boosted.netlify.app/docs/5.1/examples/title-bars/
https://deploy-preview-956--boosted.netlify.app/docs/5.1/components/title-bars/
DoD
Development
scss-docs
shortcoderebase -i
feat(…): …
messageback-from-v4
): renamed variables, changes in markup requirement, etc.Reviews