Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Load fonts and images via source-relative URLs and requires #2460

Merged
merged 6 commits into from
Jan 18, 2019

Conversation

jryans
Copy link
Collaborator

@jryans jryans commented Jan 18, 2019

As part of adding versioned URLs in element-hq/element-web#7932, we need to convert all image references to flow through the build system.

This is a change that everyone on the team should be aware of, as it affects how image paths are written. In particular:

  • CSS files now use relative paths as if you were in a theme's root source file, such as ./res/themes/dharma/css/dharma.scss
    • A convenient $(res) helper variable has been defined so we can write url('$(res)/img/cancel.svg') while still supporting themes in different places (such as Status in riot-web)
  • JS components use relative paths from a particular component's source file to the image

Needed for element-hq/element-web#7932
Used in element-hq/element-web#8159

This means that themes which include `light/css/_base.scss` (currently Dark and
Status) won't be forced to have Light's font-faces included.  This only really
matters for Status, which uses different fonts throughout.
Adds a `$res` SCSS variable set to the path from the root SCSS file to the `res`
directory.

This is a different base path than previously used in CSS URLs (it goes up 3
directories instead of 2), because Webpack will now be resolving images relative
to the root SCSS file, so the path corresponds to a source tree location,
instead of a path in the build output tree.

Defining this variable has two main goals:

* URLs are a bit easier to read
* The path can be overridden, which is needed for external (riot-web) themes
I am about to run some code replacements that might make lines too long, so with
this change I know we're starting from 0 line length warnings.
This allows Webpack to insert the proper image URL after builds steps like
adding a hash and so on. The path you supply to `require` is relative to the JS
source file, just like any other would be.
@jryans jryans changed the title Load images via source-relative URLs and requires Load fonts and images via source-relative URLs and requires Jan 18, 2019
@jryans
Copy link
Collaborator Author

jryans commented Jan 18, 2019

I think CI is confused here, so I am going to ignore it and request review. I'll make sure everything works as expected before merging.

@jryans jryans requested a review from a team January 18, 2019 05:01
@turt2live
Copy link
Member

turt2live commented Jan 18, 2019

The /pr build is failing because it first saw the commits on #2459 and is trying to pull those in still. The PR was closed before it finished the build, so the refs are all missing. The /push build on the other hand seems to have reams of errors relating to what looks to be it trying to parse PNGs as JS.

Edit: to fix the /pr build, throw a commit on the stack. An empty one would work, but isn't the best history-wise.

@jryans
Copy link
Collaborator Author

jryans commented Jan 18, 2019

Removing review request for now. There are still a few issues with Karma tests to fix.

@jryans jryans removed the request for review from a team January 18, 2019 15:10
@jryans jryans requested a review from a team January 18, 2019 17:06
@jryans
Copy link
Collaborator Author

jryans commented Jan 18, 2019

Karma issues resolved and CI is green now.

@turt2live turt2live self-assigned this Jan 18, 2019
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

looks like a couple unrelated linting fixes landed here too, but they also look reasonable.

@turt2live turt2live assigned jryans and unassigned turt2live Jan 18, 2019
@jryans jryans merged commit 7c9509c into experimental Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants