-
Notifications
You must be signed in to change notification settings - Fork 166
Extract component-specific stylesheets #6168
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
Changes from all commits
8e85fe5
df2e1b3
db59bff
cfbe98c
d58c71f
c3a62f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,41 @@ | ||
| class BaseComponent < ViewComponent::Base | ||
| def before_render | ||
| return if @rendered_scripts | ||
| @rendered_scripts = true | ||
| if helpers.respond_to?(:enqueue_component_scripts) && self.class.scripts.present? | ||
| helpers.enqueue_component_scripts(*self.class.scripts) | ||
| end | ||
| render_assets unless rendered_assets? | ||
| end | ||
|
|
||
| def self.scripts | ||
| @scripts ||= _sidecar_files(['js', 'ts']).map { |file| File.basename(file, '.*') } | ||
| @scripts ||= sidecar_files_basenames(['js', 'ts']) | ||
| end | ||
|
|
||
| def self.stylesheets | ||
| @stylesheets ||= sidecar_files_basenames(['scss']) | ||
| end | ||
|
|
||
| def unique_id | ||
| @unique_id ||= SecureRandom.hex(4) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| class << self | ||
| def sidecar_files_basenames(extensions) | ||
| _sidecar_files(extensions).map { |file| File.basename(file, '.*') } | ||
| end | ||
| end | ||
|
|
||
| def render_assets | ||
| if helpers.respond_to?(:enqueue_component_scripts) && self.class.scripts.present? | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see it was in the old code, but can you remind me why we need to do a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
A bit of speculative thinking that these components could be used outside Rails, based on #5398 (comment) , where |
||
| helpers.enqueue_component_scripts(*self.class.scripts) | ||
| end | ||
|
|
||
| if helpers.respond_to?(:enqueue_component_stylesheets) && self.class.stylesheets.present? | ||
| helpers.enqueue_component_stylesheets(*self.class.stylesheets) | ||
| end | ||
|
|
||
| @has_rendered_assets = true | ||
|
aduth marked this conversation as resolved.
|
||
| end | ||
|
|
||
| def rendered_assets? | ||
| @has_rendered_assets | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| # rubocop:disable Rails/HelperInstanceVariable | ||
| module StylesheetHelper | ||
| def stylesheet_tag_once(*names) | ||
| @stylesheets ||= [] | ||
| @stylesheets |= names | ||
| nil | ||
| end | ||
|
|
||
| alias_method :enqueue_component_stylesheets, :stylesheet_tag_once | ||
|
|
||
| def render_stylesheet_once_tags | ||
| return if @stylesheets.blank? | ||
| safe_join(@stylesheets.map { |stylesheet| stylesheet_link_tag(stylesheet) }) | ||
| end | ||
| end | ||
| # rubocop:enable Rails/HelperInstanceVariable |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| @import '@18f/identity-idp-required-scss'; | ||
|
|
||
| .troubleshooting-options { | ||
| @include u-margin-y(4); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| @import 'identity-style-guide/dist/assets/scss/packages/required'; | ||
| @import '@18f/identity-idp-required-scss'; | ||
| @import '@18f/identity-components/troubleshooting-options'; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels a little fragile to have to pull in stylesheets based on the components being used in this pack. Also, this is duplicating the styles into the |
||
| @import './components/acuant-capture'; | ||
| @import './components/acuant-capture-canvas'; | ||
| @import './components/form-steps'; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| # `@18f/identity-idp-required-scss` | ||
|
|
||
| Minimal Sass configuration and mixins required for any stylesheet which intends to make use of [Login.gov Design System](https://design.login.gov/) utilities. This contains only configuration which is specific to the IdP application, and any program-wide configuration should reside in the [design system codebase](https://github.com/18F/identity-style-guide). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| @import 'identity-style-guide/dist/assets/scss/packages/required'; | ||
|
|
||
| $theme-body-font-size: 'sm'; | ||
| $theme-font-path: '.'; | ||
| $theme-image-path: 'identity-style-guide/dist/assets/img'; | ||
| $theme-global-border-box-sizing: true; | ||
| $theme-global-link-styles: true; | ||
| $theme-grid-container-max-width: 'tablet-lg'; | ||
| $theme-header-min-width: 'tablet'; | ||
| $theme-link-visited-color: 'primary'; | ||
| $theme-style-body-element: true; | ||
| $theme-utility-breakpoints: ( | ||
| 'card': false, | ||
| 'card-lg': false, | ||
| 'mobile': false, | ||
| 'mobile-lg': false, | ||
| 'tablet': true, | ||
| 'tablet-lg': false, | ||
| 'desktop': false, | ||
| 'desktop-lg': false, | ||
| 'widescreen': false, | ||
| ); | ||
|
|
||
| $line-height: 1.5; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file organization is unsustainable, but I think we should embrace the unsustainability to force us to improve how these are managed: Namely, the typographical settings should be applied universally at the design system. |
||
|
|
||
| $h1: 1.5rem; | ||
| $h2: 1.25rem; | ||
| $h3: 1.125rem; | ||
| $h4: 1rem; | ||
| $h5: 0.875rem; | ||
| $h6: 0.75rem; | ||
|
|
||
| $sm-h1: 1.75rem; | ||
| $sm-h2: 1.375rem; | ||
| $sm-h3: 1.125rem; | ||
| $sm-h4: 1rem; | ||
| $sm-h5: 0.875rem; | ||
| $sm-h6: 0.75rem; | ||
|
|
||
| %h1 { | ||
| line-height: 1.35; | ||
| font-size: $h1; | ||
|
|
||
| @include at-media('tablet') { | ||
| font-size: $sm-h1; | ||
| } | ||
| } | ||
|
|
||
| %h2 { | ||
| line-height: 1.35; | ||
| font-size: $h2; | ||
|
|
||
| @include at-media('tablet') { | ||
| font-size: $sm-h2; | ||
| } | ||
| } | ||
|
|
||
| %h3 { | ||
| line-height: $line-height; | ||
| font-size: $h3; | ||
|
|
||
| @include at-media('tablet') { | ||
| font-size: $sm-h3; | ||
| } | ||
| } | ||
|
|
||
| %h4 { | ||
| line-height: $line-height; | ||
| font-size: $h4; | ||
|
|
||
| @include at-media('tablet') { | ||
| font-size: $sm-h4; | ||
| } | ||
| } | ||
|
|
||
| %h5 { | ||
| line-height: $line-height; | ||
| font-size: $h5; | ||
|
|
||
| @include at-media('tablet') { | ||
| font-size: $sm-h5; | ||
| } | ||
| } | ||
|
|
||
| %h6 { | ||
| line-height: $line-height; | ||
| font-size: $h6; | ||
|
|
||
| @include at-media('tablet') { | ||
| font-size: $sm-h6; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| { | ||
| "name": "@18f/identity-idp-required-scss", | ||
| "private": true, | ||
| "version": "1.0.0", | ||
| "peerDependencies": { | ||
| "identity-style-guide": "*" | ||
| } | ||
| } |
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.
Technically, this behavior is unused, and is a remnant of early implementation approach. That being said, it feels like it's a reasonably expected behavior to align with how
self.scriptsis implemented, if we're going to support bothself.scriptsandself.stylesheetsregardless (even ifself.stylesheetswould be implemented by the individual component).