Skip to content

CV-112348 Support dynamic public path and RTL fetching #1

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

Merged
merged 4 commits into from
Nov 20, 2019

Conversation

SimulatedGREG
Copy link

@SimulatedGREG SimulatedGREG commented Nov 19, 2019

This PR contains a:

  • new feature

Motivation / Use-Case

This PR implements 2 new options for this plugin.

  • globalRTLFlag
    Adds the ability to check a global window flag at run-time to determine if a RTL stylesheet should be fetched.
  • outputPublicPath
    Adds the ability to manually set the request's public path, no longer requiring all requests to match webpack's output.publicPath setting.

Comment on lines +337 to +339
'if (fetchRTL) {',
Template.indent([
`fullhref = fullhref.replace(/\\.css/i, '.rtl.css');`,
Copy link
Author

Choose a reason for hiding this comment

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

Replace .css extension with .rtl.css if fetchRTL is enabled.

Copy link

@danab danab left a comment

Choose a reason for hiding this comment

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

It looks weird to me, but so do most webpack plugins.

Is it worth adding some sort of manual tests to that page? Probably not...

@@ -5,7 +5,7 @@
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<title>mini-css-extract-plugin testcase</title>
<meta name="viewport" content="width=device-width, initial-scale=1">
<link rel="stylesheet" type="text/css" href="/dist/preloaded1.css" />
<link rel="stylesheet" type="text/css" href="/dist/preloaded1.client.css" />
Copy link

Choose a reason for hiding this comment

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

This gets a mime/type error when testing, but it is the same on master, so I guess that's a potential PR to the main repo.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, it has to do with the webpack-dev-server settings. Nothing to worry about though.

filename: '[name].client.css',
chunkFilename: '[name].chunk.client.css',
globalRTLFlag: 'rtlLanguageEnabled',
// outputPublicPath: '/bundles/css/',
Copy link

Choose a reason for hiding this comment

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

Suggested change
// outputPublicPath: '/bundles/css/',

Copy link
Author

Choose a reason for hiding this comment

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

This only pertains to the testing environment. Decided to keep this comment for testing later if things need to be adjusted or if the fork needs to pull in changes from the master origin.

@SimulatedGREG SimulatedGREG merged commit dd7cf65 into master Nov 20, 2019
@SimulatedGREG SimulatedGREG deleted the GH/CV-112348-support-dynamic-public-path-rtl branch November 20, 2019 18:21
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

Successfully merging this pull request may close these issues.

2 participants