Skip to content

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

Merged
merged 27 commits into from
Apr 8, 2024
Merged

Onboarding components fixes #1393

merged 27 commits into from
Apr 8, 2024

Conversation

levkk
Copy link
Contributor

@levkk levkk commented Apr 3, 2024

New components

  1. Gray heading.
  2. Primary card that doesn't break absolute-positioned elements (dropdown) rendered in other cards.

Bug fixes

  1. Radio input correctly notifies caller in case of change.
  2. Small table shouldn't have a hover effect.

Small additions

  1. Added optional anchor to SwitchV2. Helps with navigation using Turbo frames or streams.
  2. A few helpful setters.
  3. Split up unit displayed in the input and unit displayed in the cost estimate in RangeGroupV2
  4. More constructors for StimulusAction to avoid importing enum.

@levkk levkk requested a review from chillenberger April 4, 2024 22:33
@levkk levkk marked this pull request as ready for review April 4, 2024 22:34
@levkk levkk force-pushed the levkk-onboarding-fixes branch from b1af0e8 to e569e9f Compare April 4, 2024 22:35
Comment on lines +61 to +64
pub fn h_100(mut self) -> Self {
self.controller_classes.push("h-100".into());
self.card_classes.push("h-100".into());
self
Copy link
Contributor

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.

Copy link
Contributor Author

@levkk levkk Apr 5, 2024

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.

Comment on lines +19 to +22
pub fn z_index(mut self, index: i64) -> Self {
self.style = format!("position: relative; z-index: {};", index);
self
}
Copy link
Contributor

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?

Copy link
Contributor Author

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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Comment on lines +1 to +3
span[data-controller="headings-gray"] {
color: #{$gray-400};
}
Copy link
Contributor

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.

@levkk levkk merged commit ee7e83e into master Apr 8, 2024
@levkk levkk deleted the levkk-onboarding-fixes branch April 8, 2024 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants