Skip to content

Support scss absolute path resolution for url() - #7937

Merged
mrmckeb merged 2 commits into
react:masterfrom
atlanteh:patch-1
Jun 7, 2020
Merged

Support scss absolute path resolution for url()#7937
mrmckeb merged 2 commits into
react:masterfrom
atlanteh:patch-1

Conversation

@atlanteh

@atlantehatlanteh commented Nov 6, 2019

Copy link
Copy Markdown
Contributor

Adding resolve-url-loader broke all apps using scss with centralized assets folder and all url(./assets/*.png) broke (#7023)
This change allows apps to use url(/assets/*.png) and it would map to src/assets/*.png

I tested this on my app which has an images directory and after upgrading to CRA 3.2.0 (from 2.1.2) it failed. After reading resolve-url-loader I added this option and changed the path to absolute (/assets/*.png) and everything started working again

@ulrichb

ulrichb commented Nov 6, 2019

Copy link
Copy Markdown

... build error (Cannot find module '@babel/helper-create-regexp-features-plugin') seems to be unrelated with the change in this PR.

@atlanteh

atlanteh commented Nov 6, 2019

Copy link
Copy Markdown
ContributorAuthor

Yes, it's not related. Build is broken for the last 24 hours

@atlanteh

atlanteh commented Nov 12, 2019

Copy link
Copy Markdown
ContributorAuthor

@skrzepij thanks for approving the PR!
When can we expect this to land?

@atlanteh

atlanteh commented Nov 27, 2019

Copy link
Copy Markdown
ContributorAuthor

any update on this? We can't run CI ever since this issue occurred as we need this fix in order for CI to work properly

@mrmckeb

mrmckeb commented Dec 1, 2019

Copy link
Copy Markdown
Contributor

@atlanteh Can you please rebase and push up again - the CI should work now.

@atlanteh

atlanteh commented Dec 1, 2019

Copy link
Copy Markdown
ContributorAuthor

@mrmckeb All passed :)

@mrmckeb

mrmckeb commented Dec 1, 2019

Copy link
Copy Markdown
Contributor

I'm OK with this change, @ianschmitz or @iansu - any thoughts?

@atlanteh

atlanteh commented Dec 5, 2019

Copy link
Copy Markdown
ContributorAuthor

@mrmckeb@ianschmitz@iansu
If there are no objection, can we proceed with this PR?

@atlanteh

atlanteh commented Dec 17, 2019

Copy link
Copy Markdown
ContributorAuthor

Any update?

@atlanteh

atlanteh commented Dec 21, 2019

Copy link
Copy Markdown
ContributorAuthor

??

@atlanteh

atlanteh commented Jan 7, 2020

Copy link
Copy Markdown
ContributorAuthor

Can this PR be merged please?

@atlanteh

atlanteh commented Jan 12, 2020

Copy link
Copy Markdown
ContributorAuthor

@skrzepij@mrmckeb@ianschmitz@iansu
Can you please reply? Why is there a delay in this? This PR was created over two months ago. It's a basic 3 words fixup.
Please let's get this merged

@mrmckebmrmckeb added this to the 3.4 milestone Jan 13, 2020
@mrmckeb

mrmckeb commented Jan 13, 2020

Copy link
Copy Markdown
Contributor

I've tagged this for 3.4 so someone should review and merge it before that release. Sorry for the delay, a lot of the team are only returning after Christmas break.

@atlanteh

atlanteh commented Jan 13, 2020

Copy link
Copy Markdown
ContributorAuthor

Thanks!

@iansuiansu modified the milestones: 3.4, 3.5Feb 14, 2020
@atlanteh

atlanteh commented Feb 23, 2020

Copy link
Copy Markdown
ContributorAuthor

Can you guys make sure this gets into next release? We really need this fix, and we've been working with this fix locally for 3 months now and it works great

@atlanteh

atlanteh commented Apr 23, 2020

Copy link
Copy Markdown
ContributorAuthor

Why is this moved to milestone 3.6? This is a very small change. This PR is waiting for almost 6 months now. Please merge it 🙏

@arkn98

arkn98 commented Apr 29, 2020

Copy link
Copy Markdown

+1 Please get this merged.

@arkn98arkn98 mentioned this pull request May 29, 2020
Closed
@arwagner

arwagner commented Jun 5, 2020

Copy link
Copy Markdown

Need this fix merged ASAP, please.

@mrmckeb

mrmckeb commented Jun 5, 2020

Copy link
Copy Markdown
Contributor

I'm really sorry this one slipped through. We had a quick chat and we think it's good to go, but we'd like to see a quick test added if that's OK @atlanteh?

@atlanteh
Support scss absolute path resolution for url()
2dd16ab
Adding resolve-url-loader broke all apps using scss with centralized assets folder and all url(./assets/*.png) broke (#7023).
This change allows apps to use url(/assets/*.png) and it would map to src/assets/*.png
@atlanteh
test: Add global scss assets test
eda3a66
@atlanteh

atlanteh commented Jun 6, 2020

Copy link
Copy Markdown
ContributorAuthor

@mrmckeb I added a test, though I see master is currently red, so my tests will fail as well.
I pretty much copy-pasted the bootstrap test and changed what is required. Both these tests cannot run locally due to node-sass is not installed exception, but I see that they pass in the behavior pipelines

mrmckeb
mrmckeb reviewed Jun 7, 2020
View reviewed changes

@mrmckebmrmckeb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@atlanteh The SVG seems odd to me and isn't loading - can you use the logo from our templates?

Or just use a minimal SVG that's just a box? Otherwise I this looks good.

@atlanteh

atlanteh commented Jun 7, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@mrmckeb This is the logo from the templates.. What do you mean by "It isn't loading"? Are you running the tests locally and you can see the results? If so, how?

@mrmckeb

mrmckeb commented Jun 7, 2020

Copy link
Copy Markdown
Contributor

Sorry @atlanteh, I don't know what I did - it's fine, you're right. Hope you're having a great weekend, I'll get this merged in very shortly.

@mrmckeb
mrmckeb merged commit fa648da into react:masterJun 7, 2020
victor-travelperk pushed a commit to travelperk/create-react-app that referenced this pull request Jun 8, 2020
@atlanteh@victor-travelperk
Add support for absolute paths when using url() in CSS (react#7937)
8fa4e28
* Support scss absolute path resolution for url()
Adding resolve-url-loader broke all apps using scss with centralized assets folder and all url(./assets/*.png) broke (react#7023).
This change allows apps to use url(/assets/*.png) and it would map to src/assets/*.png
* test: Add global scss assets test
@atlanteh
atlanteh deleted the patch-1 branch June 9, 2020 07:39
@locklockBot locked and limited conversation to collaborators Jun 24, 2020
@iansuiansu modified the milestones: 4.1, 4.0Sep 16, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@iansuiansuAwaiting requested review from iansu
@petetntpetetntAwaiting requested review from petetnt
3 more reviewers
@skrzepijskrzepijskrzepij approved these changes
@mrmckebmrmckebmrmckeb approved these changes
@ianschmitzianschmitzianschmitz approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed

Projects

None yet

Milestone

4.0

Development

Successfully merging this pull request may close these issues.

9 participants

@atlanteh@ulrichb@mrmckeb@arkn98@arwagner@skrzepij@ianschmitz@iansu@facebook-github-bot