-
Notifications
You must be signed in to change notification settings - Fork 21
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
Added new Hero variant Pale Blue Bubbles #3476
base: main
Are you sure you want to change the base?
Added new Hero variant Pale Blue Bubbles #3476
Conversation
✅ Deploy Preview for ons-design-system-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Really only one comment but crops up in a few places. I didn't bother to add the same comment to the tests but they would also need to be updated.
src/components/hero/_hero.scss
Outdated
@@ -28,6 +28,26 @@ | |||
} | |||
} | |||
|
|||
&--topic-grey { |
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.
I think topic-grey
isn't the best name here as this example is only called topic
because of the text supplied to the topic option. This could be anything. Perhaps grey-bubbles
or something might be better?
src/components/hero/_hero.scss
Outdated
@@ -329,6 +349,77 @@ | |||
} | |||
} | |||
|
|||
&--topic-grey & { |
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.
Same comment as above:
I think topic-grey
isn't the best name here as this example is only called topic
because of the text supplied to the topic option. This could be anything. Perhaps grey-bubbles
or something might be better?
src/components/hero/_macro.njk
Outdated
@@ -13,8 +13,17 @@ | |||
{% endfor %} | |||
</div> | |||
{% endif %} | |||
{% if params.variants == 'topic-grey' %} |
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.
Same comment as above:
I think topic-grey isn't the best name here as this example is only called topic because of the text supplied to the topic option. This could be anything. Perhaps grey-bubbles or something might be better?
} | ||
] | ||
}, | ||
"variants": 'topic-grey' |
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.
Same comment as above:
I think topic-grey isn't the best name here as this example is only called topic because of the text supplied to the topic option. This could be anything. Perhaps grey-bubbles or something might be better?
Actually the main colour of this hero is pale blue so should we name it something closer to that rather than grey? |
I think we need to update the name of this component to light or pale blue rather than grey. To me the hero looks blue not grey |
Shall I change variant name as well? |
Yeah I think so, anywhere the variant name is used |
What is the context of this PR?
ONSDESYS-164 Added new Hero as per the ticket.
New Variant will have Breadcrumbs, word "Topic" which is configurable, a sub-header and corresponding text.
How to review this PR
Check that new example added matches the design as in figma and the available prototype
Checklist
This needs to be completed by the person raising the PR.