-
Notifications
You must be signed in to change notification settings - Fork 13
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
Display CTA component with a slot #345
Conversation
✅ Deploy Preview for bump-content-hub 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.
I left a suggestion concerning the CSS, but otherwise this seems good to me!
src/_components/shared/sidebar.css
Outdated
@@ -41,6 +41,7 @@ side-bar { | |||
display: flex; | |||
flex-direction: column; | |||
gap: var(--spacing-1); | |||
margin-bottom: var(--spacing-8); /* please don't trust me here */ |
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 it would be better to put this margin directly on the cta, since this margin should only exist when the cta follows the sidebar-list.
It would look like this:
.sidebar-list + bump-cta {
margin-block-start: var(--spacing-8);
}
Here we favour the usage of the logical property margin-block-start
(which is similar to margin-top
as it would be more flexible in case of using languages having different reading directions.
Also, I think it would be better to comment directly in github when submitting your PR instead of adding the comment in the code. This way it opens the discussion, and in case the code is alright, we don't have an irrelevant message to remove. 😊
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 it would be better to put this margin directly on the cta, since this margin should only exist when the cta follows the sidebar-list.
Indeed it looks really more clear with your suggestion, thanks for the explanation
Also, I think it would be better to comment directly in github when submitting your PR instead of adding the comment in the code. This way it opens the discussion, and in case the code is alright, we don't have an irrelevant message to remove. 😊
Sure 😄
In this case I was quite sure my CSS was not enough, ne need to merge this in first state, so I was quite sure to remember to remove the comment.
Purpose of this PR is to display the CTA button in guides section 'OpenAPI specification'. I noticed that component `Shared::BumpCta` was already included in layout documentation.erb, as a slot named `:after` in component `Shared::Sidebar`, but it was not displayed. What is done in this PR: - change condition to display this component: favor ready-to-use boolean `resource.data.display_cta` - render the slot in the component, with `<%= slotted :after %>` - add basic CSS to keep space between this CTA and the sidebar links 😇 (no dinguerie I swear!) - set data.slug to 'openapi-specification', used as `utm_content`.
2a813df
to
f61aae0
Compare
Purpose of this PR is to display the CTA button in guides section 'OpenAPI specification'.
I noticed that component
Shared::BumpCta
was already included in layout_documentation.erb
, as a slot named:after
in componentShared::Sidebar
, but it was not displayed.What is done in this PR:
change condition to display this component: favor ready-to-use boolean
resource.data.display_cta
render the slot in the component, with
<%= slotted :after %>
add basic CSS to keep space between this CTA and the sidebar links 😇 (no dinguerie I swear!)
set data.slug to 'openapi-specification', used as
utm_content
.ℹ️ Data
display_cta
is responsible to display the BumpCta component. We would need to change value to false where it's not necessary.Now:

context: Slack discussion