Skip to content
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

Add components import to theme/styles.scss #3117

Closed
saracope opened this issue Sep 24, 2019 · 14 comments
Closed

Add components import to theme/styles.scss #3117

saracope opened this issue Sep 24, 2019 · 14 comments

Comments

@saracope
Copy link
Contributor

Description

I pulled down the v2.2.1 update this morning and had to change the path for ../img/hero.png in the settings in order to get it working. I updated it to {$theme-image-path}/hero.png so it automatically grabs whatever image path I have in my theme settings.

I did this in my _uswds-theme-general.scss file which is fine and not unexpected but I also had to make the change in uswds/scss/settings/_settings-components.scss to fix the error I was getting. (screenshots below)

It seems that I would need to update _settings-components.scss every time a new USWDS version is released (we run a script to remove and then copy the entire updated USWDS package over to our file structure) unless we can incorporate using the theme variable for image paths as a global change (like in screenshot 3).

Wanted to get others thoughts on this and make sure this isn't just an issue created by how our files are structured. Here's the branch I'm working on.

Additional information [optional]

Screen Shot 2019-09-24 at 10 56 30 AM

2.

Screen Shot 2019-09-24 at 10 55 17 AM

3.

Screen Shot 2019-09-24 at 10 55 05 AM

@thisisdano
Copy link
Member

Lemme see if I understand — since there's a related issue around this also open (#3116).

Are you saying that changing the value of $theme-hero-image in _uswds-theme-components.scss is not enough to change the value in the code — that you also need to change it in _settings-components.scss?

One thing we can and should do is change to default value of $theme-hero-image to '{$theme-image-path}/hero.png' so there's less chance that upgrading will break existing usage. However, you should not have to use (or ever touch) the values in the settings directory — those are intended to set the internal defaults only, and not be user-settable. If you find you're having to adjust those values, there's something wrong.

@thisisdano
Copy link
Member

thisisdano commented Sep 24, 2019

The proper formatting for that variable should be '#{$theme-image-path}/hero.png' — with the # sign. Maybe that'll make a difference?

@saracope
Copy link
Contributor Author

With some more debugging, I saw that the uswds-theme-components file wasn't getting processed at all.

I added an import for uswds-theme-components in theme > styles.scss. Once I added it there, it compiled w/o needing the change in settings.

Screen Shot 2019-09-25 at 10 57 46 AM

I don't see where else that would get processed. Should I be looking at something else?

@thisisdano
Copy link
Member

Aha! It looks like that theme import statement is missing from the theme's styles.scss file. We'll need to add that one in there. Looks like that's been a bug/issue since the 2.0.0 release.

I'm going to edit the title of this issue and make it about adding that import to theme/styles.scss — and a fix will go out in the next release.

@thisisdano
Copy link
Member

But the fix you made should fix your problem.

@thisisdano thisisdano changed the title Use $theme-hero-image variable in Settings Add components import to theme/styles.scss Sep 25, 2019
@saracope
Copy link
Contributor Author

Ok, thanks!

@mejiaj
Copy link
Contributor

mejiaj commented Oct 30, 2019

@thisisdano , w/ #3119 merged should we close this issue?

@adborden
Copy link
Contributor

I just pulled v2.3.0 but I'm still seeing this error.

@adborden
Copy link
Contributor

Sorry, should've included more detail... Maybe this is a different issue, let me know and I will open a new one. From a slack conversation, I was under the impression my error was caused by the missing import.

Running gulp init results in an undefined variable $theme-navigation-width.

[15:34:55] Starting 'build-sass'...
[15:34:56] 'build-sass' errored after 1.32 s
[15:34:56] Error in plugin "gulp-sass"
Message:
    pages/_scss/components/_header.scss
Error: Undefined variable: "$theme-navigation-width".
        on line 16 of pages/_scss/components/_header.scss
        from line 33 of pages/_scss/_uswds-theme-custom-styles.scss
        from line 36 of pages/_scss/styles.scss
>>   @include at-media($theme-navigation-width) {

   --------------------^

Details:
    status: 1
    file: /home/gsa/projects/datagov/resources.data.gov/pages/_scss/components/_header.scss
    line: 16
    column: 21
    formatted: Error: Undefined variable: "$theme-navigation-width".
        on line 16 of pages/_scss/components/_header.scss
        from line 33 of pages/_scss/_uswds-theme-custom-styles.scss
        from line 36 of pages/_scss/styles.scss
>>   @include at-media($theme-navigation-width) {

   --------------------^

    messageFormatted: pages/_scss/components/_header.scss
Error: Undefined variable: "$theme-navigation-width".
        on line 16 of pages/_scss/components/_header.scss
        from line 33 of pages/_scss/_uswds-theme-custom-styles.scss
        from line 36 of pages/_scss/styles.scss
>>   @include at-media($theme-navigation-width) {

   --------------------^

    messageOriginal: Undefined variable: "$theme-navigation-width".
    relativePath: pages/_scss/components/_header.scss
    domainEmitter: [object Object]
    domain: [object Object]
    domainThrown: false

[15:34:56] 'init' errored after 1.68 s

@thisisdano
Copy link
Member

Hm. OK — I'll look at this again when I have a sec.

@thisisdano
Copy link
Member

As you're using gulp init — is this connected to uswds-gulp?

@adborden
Copy link
Contributor

Thanks for looking into it. Yes, we're using uswds-gulp. The repo is here: https://github.com/GSA/resources.data.gov

@thisisdano
Copy link
Member

OK @adborden sorry for the delay.

2.2.0 deprecated the $theme-navigation-width var.

Replace each instance of it with $theme-header-min-width. Perhaps I should add an aliased version of the old var (ie something like $theme-navigation-width: $theme-header-min-width for backward compatibility.

@thisisdano
Copy link
Member

thisisdano commented Dec 7, 2019

Added an issue around backward compatibility: #3254

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

No branches or pull requests

4 participants