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

Pluggable cities assets #687

Merged
merged 8 commits into from Jun 14, 2019
Merged

Conversation

frwickst
Copy link
Contributor

@frwickst frwickst commented Jun 12, 2019

So this was a fun one, lots of trial and error, in the beginning, to figure out the nicest way to do this. So what have I done?

What does it do?

The code enables other cities to create their own assets and themes for kerrokantasi. It removes as much as possible of the Helsinki base assets from the codebase and instead moves all Helsinki related things to /cities/assets. Since I did not want to create any disruption for the Helsinki devs, the Helsinki themes/assets got to live on in the same repository. However, the main point would be that themes/assets would/could be separated into a separate project and then be activated by installing them and changing only a single config parameter, city_config.

To keep the application working as it did before, things will default to the Helsinki styles and assets and everything should work as it did before the change. However, it is now easy to load the whitelabel styles by settings city_config = "whitelabel" or to change to a different cities theme/assets, for instance city_config = "kerrokantasi-ui-turku" (https://github.com/City-of-Turku/kerrokantasi-ui-turku).

How was it done?

Mainly by setting correct aliases in the WebPack. This allowed me to load assets from different places in the code base. This also allowed for cross-importing things from kerrokantasi-ui into a theme/assets package. When all of that was done, it was mainly figuring out where things should go and moving things around and pointing to the correct files in the code.

Docs

Documentation regarding the creation of themes/asset packages can be found in the README so I won't bother explaining them here.

Notes

  1. This does not remove all Helsinki related things from the main codebase yet. There are still some hardcoded strings that are left, and for instance, authentication is still very much Helsinki specific. This is mainly the first step into making the project more city agnostic so that others can use the same codebase and just be able to change the styling and strings that they want to.

  2. As can be noted in the readme under "Installing city specific assets" and then "Development steps", there are still some caveats to this when creating external packages that would be nice to fix, but due to time constraints these were unfortunately not figured out.

@frwickst frwickst force-pushed the pluggable-cities branch 2 times, most recently from 85d7ee2 to e4be982 Compare June 12, 2019 13:23
@codecov-io
Copy link

codecov-io commented Jun 12, 2019

Codecov Report

Merging #687 into master will increase coverage by <.01%.
The diff coverage is 14.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #687      +/-   ##
==========================================
+ Coverage   24.16%   24.17%   +<.01%     
==========================================
  Files         134      135       +1     
  Lines        2884     2887       +3     
  Branches      491      490       -1     
==========================================
+ Hits          697      698       +1     
- Misses       1813     1816       +3     
+ Partials      374      373       -1
Impacted Files Coverage Δ
src/index.js 0% <ø> (ø) ⬆️
server/getSettings.js 0% <ø> (ø) ⬆️
src/components/Header/index.js 57.14% <ø> (ø) ⬆️
src/App.js 0% <ø> (ø) ⬆️
src/components/Footer.js 0% <0%> (ø) ⬆️
server/bootstrap.js 0% <0%> (ø) ⬆️
...ws/FullscreenHearing/FullscreenHearingContainer.js 0% <0%> (ø) ⬆️
src/i18n/index.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 132f330...e65c518. Read the comment docs.

Copy link
Contributor

@Rikuoja Rikuoja left a comment

Choose a reason for hiding this comment

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

Looks sensible!

Perhaps add the city_config variable to config_dev.toml.example too, with explanations?

@Rikuoja Rikuoja requested a review from vikoivun June 13, 2019 11:20
@Rikuoja
Copy link
Contributor

Rikuoja commented Jun 13, 2019

I' m asking @vikoivun to comment on this one too, because he will be the one responsible for deployment anyway :) Everything should work as before, but better that he knows what is happening.

@frwickst
Copy link
Contributor Author

Added city_config to config_dev.toml.example.

@Rikuoja
Copy link
Contributor

Rikuoja commented Jun 14, 2019

Looks like everybody is happy with this one, so I'm merging.

@Rikuoja Rikuoja merged commit debd3ee into City-of-Helsinki:master Jun 14, 2019
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.

None yet

4 participants