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.

Sign up for GitHub

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

Jump to bottom

Fix sass :export keyword #20201

Closed
wants to merge 1 commit into from
Closed

Fix sass :export keyword #20201

wants to merge 1 commit into from

Conversation

davidde
Copy link

@davidde davidde commented Dec 18, 2019

Description

This change fixes gatsby-plugin-sass.
Variables exported with the sass :export keyword are currently undefined on the initial page load in production (but work fine in development).

Related Issues

Fixes issue #19563
Related to #10706

@davidde davidde requested a review from a team as a code owner December 18, 2019 21:45
pvdz
pvdz approved these changes Dec 19, 2019
View reviewed changes
Copy link
Contributor

@pvdz pvdz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether this needs to be repeated but that's benign right now.

Thanks for fixing this! :D

@pvdz pvdz self-requested a review December 19, 2019 08:51
@pvdz
Copy link
Contributor

pvdz commented Dec 19, 2019

Looks like snapshots need to be updated.

Please run yarn jest -u to update snapshots. Check the diff to see if anything looks unexpected. Otherwise please add them to the PR :)

@wardpeet wardpeet added breaking change If implemented, this proposed work would break functionality for older versions of Gatsby status: blocked This issue/PR can't be solved at the moment and shouldn't be closed/merged and removed breaking change If implemented, this proposed work would break functionality for older versions of Gatsby labels Dec 19, 2019
@wardpeet
Copy link
Contributor

Hey @davidde,

The change you’ve proposed is not going to be accepted because it changes behavior and might lead to unwanted bugs. CSS does not have the :export, :local, :global syntax. This syntax is part of the css-modules spec. If you change your .css files into .module.css it will work as the loader will use the specific css-modules settings.

More info can be found here https://github.com/css-modules/css-modules

We absolutely want to have you as a contributor, so please take a look at our open issues for ideas, and please reach out to us on Twitter at @gatsbyjs with questions.

We offer pair programming sessions if you’d like to work with one of our maintainers to make another contribution.

Thanks again, and we look forward to seeing more PRs from you in the future! 💪💜

@wardpeet wardpeet closed this Dec 19, 2019
@davidde
Copy link
Author

davidde commented Dec 19, 2019

Thank you for the elaborate explanation and encouraging words. Much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pvdz pvdz Awaiting requested review from pvdz

Assignees
No one assigned
Labels
status: blocked This issue/PR can't be solved at the moment and shouldn't be closed/merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@davidde @pvdz @wardpeet