Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
levkk merged 27 commits intomasterfromlevkk-onboarding-fixes
Apr 8, 2024
Merged

Onboarding components fixes#1393

levkk merged 27 commits intomasterfromlevkk-onboarding-fixes
Apr 8, 2024

Conversation

levkk
Copy link
Contributor

@levkklevkk commentedApr 3, 2024
edited
Loading

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 toSwitchV2. 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 inRangeGroupV2
  4. More constructors forStimulusAction to avoid importing enum.

@levkklevkk marked this pull request as ready for reviewApril 4, 2024 22:34
@levkklevkkforce-pushed thelevkk-onboarding-fixes branch fromb1af0e8 toe569e9fCompareApril 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.

levkk reacted with thumbs up emoji
Copy link
ContributorAuthor

@levkklevkkApr 5, 2024
edited
Loading

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>. 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
ContributorAuthor

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
ContributorAuthor

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 reacted with thumbs up emoji
@levkklevkk merged commitee7e83e intomasterApr 8, 2024
@levkklevkk deleted the levkk-onboarding-fixes branchApril 8, 2024 05:47
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@chillenbergerchillenbergerchillenberger left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@levkk@chillenberger

[8]ページ先頭

©2009-2025 Movatter.jp