-
Notifications
You must be signed in to change notification settings - Fork 324
Onboarding components fixes #1393
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
Conversation
b1af0e8
to
e569e9f
Compare
pub fn h_100(mut self) -> Self { | ||
self.controller_classes.push("h-100".into()); | ||
self.card_classes.push("h-100".into()); | ||
self |
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.
for something like this is it possible to just always have the h-100 on and for instances where you need to limit the height, place it in a wrapper that does not have h-100? Just brainstorming, this could turn into a slippery slope of having a function for every specific situation. I think if it is possible to solve problems like this with wrappers, we should.
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.
By default, the card occupies as much space as it needs to render its contents. The .h-100
here helps cards look uniform when placed inside a grid. Our cards don't have the same contents, so their heights will vary when placed inside a grid like .row
or .d-flex
. If we have .h-100
by default, the card will always occupy the full height of its container which is not desired when the card is used by itself inside something like a <div class="container">
. So .h-100
is a special case that we just happen to use quite often, but not always.
A wrapper can't make its child occupy 100% of its space unless we add additional CSS to the wrapper knowing that the child needs to do this. For example, here we could do something like:
.card-wrapper-h-100 {
@extend .h-100
.card {
@extend .h-100
.card-body {
@extend .h-100
}
}
}
which now that I'm writing this isn't too bad. One issue with this is more of a theoretical technicality, where a component "reaches inside" another component and modifies its state which could cause weird bugs later on. It's typically best to have the component use public methods to modify its state in a predicable way.
Maybe if we name this slightly differently, it could be better? I'm not leaning strongly one way or the other.
pub fn z_index(mut self, index: i64) -> Self { | ||
self.style = format!("position: relative; z-index: {};", index); | ||
self | ||
} |
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 thing as the next comment, is it possible to solve this with a wrapper?
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 issue as above. The wrapper would have to modify the style of its child.
value: String, | ||
} | ||
|
||
impl Gray { |
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.
If we want to go down this road, maybe one heading that takes a color, or has a setter function for the desired color, rather than a new component for every header.
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.
Yeah I agree, these are too small to be individual components.
span[data-controller="headings-gray"] { | ||
color: #{$gray-400}; | ||
} |
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.
You could add color = gray-400 to the typography.scss file and just use the class here. Utility classes for the gray scale is always nice.
This reverts commit 821c393.
New components
Bug fixes
Small additions
SwitchV2
. Helps with navigation using Turbo frames or streams.RangeGroupV2
StimulusAction
to avoid importing enum.