feat: adds theming to support multiple instances of the platform #1242
Conversation
| @@ -0,0 +1,41 @@ | |||
| import React, { Component } from 'react' | |||
There was a problem hiding this comment.
Lift primary application out of src/index into a standalone App so it can be wrapped in MobX observer method to ensure all children can be correctly re-rendered by MobX state changes.
| @@ -50,7 +50,6 @@ | |||
| "resolutions": { | |||
| "__NOTE__": "2021-05-28 storybook clash: https://github.com/storybookjs/storybook/issues/6505", | |||
| "webpack": "4.44.2", | |||
| "styled-components": "^4", | |||
There was a problem hiding this comment.
question Not sure what/why with this line item; removing unless someone says no…
There was a problem hiding this comment.
I think that might have been related to storybook back when all package installs were hoisted to top-level, but as we now have nmHoistingLimits: workspaces I expect fine to remove
| @@ -1,6 +1,6 @@ | |||
| import { Component } from 'react' | |||
|
|
|||
| import { Image, ImageProps } from 'rebass' | |||
| import { Image, ImageProps } from 'rebass/styled-components' | |||
There was a problem hiding this comment.
suggestion: We have a mixture of styled-components and vanilla Rebass components which use: https://emotion.sh/
Rebass does allow you to specifically use styled-component compatible variants, so we are switching everywhere to use those instead. https://rebassjs.org/guides/styled-components
We can easily reverse this decision and switch towards Emotion dropping Styled Components but this needs to be consistent!
There was a problem hiding this comment.
I remember at one time the lead frontend (Benj) had wanted to move to emotion, but as we had only just moved from material -> styled components seemed like one step too far. Would definitely be outside scope of the PR to make a switch so seems like a sensible option until such a time if we did decide (npm trends still has styled-components way more popular but I have no strong opinion on the matter otherwise)
thisislawatts
force-pushed
the
feat/adds-support-for-multiple-themes
branch
from
0f11f53
to
93a8daa
Compare
|
wauw just saw this. Super nice! Good step towards replicating 💪 |
thisislawatts
changed the title
thisislawatts
changed the title
thisislawatts
force-pushed
the
feat/adds-support-for-multiple-themes
branch
2 times, most recently
from
14183cf
to
579afb0
Compare
thisislawatts
force-pushed
the
feat/adds-support-for-multiple-themes
branch
from
ab4f643
to
2c4a9dc
Compare
thisislawatts
added
the
Review allow-preview ✅
|
Visit the preview URL for this PR (updated for commit 258d3d4): https://onearmy-next--pr1242-feat-adds-support-fo-t7edcyll.web.app (expires Mon, 20 Dec 2021 15:19:32 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
|
noticed a few missing things.
|
|
Thanks for taking a look around @davehakkens
|
|
|
@davehakkens Updated to add a variant for use on Project Kamp ✨
|
thisislawatts
force-pushed
the
feat/adds-support-for-multiple-themes
branch
from
258d3d4
to
920dd3b
Compare
| @@ -0,0 +1 @@ | |||
| <svg width="160" height="149" xmlns="http://www.w3.org/2000/svg"><g stroke="#F82F03" stroke-width="14.12" fill="none" fill-rule="evenodd" stroke-linecap="round" stroke-linejoin="round"></g></svg> | |||
There was a problem hiding this comment.
Really like idea of having an unbranded theme! Although definitely voting unbranded => wacky design a la 00s myspace
| // we have 2 different dev sites, only show this component when on one and provide select | ||
| const devSites = [ | ||
| { value: 'localhost', label: 'Dev' }, | ||
| { value: 'dev_site', label: 'Development' }, |
There was a problem hiding this comment.
Thanks for fixing this, was wondering why I hadn't seen it recently :S
| @@ -71,4 +73,14 @@ export const HowToCard = (props: IProps) => ( | |||
| </Flex> | |||
| ) | |||
|
|
|||
| function formatCountryToISOCode(str) { | |||
| if (str === 'united kingdom') return 'gb' | |||
There was a problem hiding this comment.
Is this to fix the issues seen in dev console or related to #1211?
In either way I don't seem harm in it, but am confused as to why there is such a short/explicit list of countries?
| @@ -51,7 +51,7 @@ const beforeUnload = function(e) { | |||
| } | |||
|
|
|||
| const ResearchForm = observer((props: IProps) => { | |||
| const store = useResearchStore() | |||
| const researchStore = useResearchStore() | |||
There was a problem hiding this comment.
nice to be clear, thanks for this!
There was a problem hiding this comment.
Thanks for the huge amount of work that's gone into this, I can see how extracting as much of the hardcoded styles/text as you have really goes a long way to making the site re-usable for other sites.
A couple specific points of feedback:
- Having to inject the themeProvider into every component feels a bit awkward, particularly when there's added boilerplate of having to define injected props or pass via commonStores. I know this is the pattern already inherited from the mobx setup, but I'm wondering if there might be more elegant solutions possible using either a
useContexthook or better yet a custom modified/adapteduseThemehook like in some of the examples here: https://styled-components.com/docs/advanced
I wouldn't go as far as to say it's worth refactoring anything at this stage, but might be worth testing so we can provide as an option for new components moving forward.
- Each of the current themes are pretty large in size, but contain mostly identical information. I'm expect this is mostly for the purpose of getting things to work quickly now and that they'll want freedom to diverge more in the future, but equally I think there's probably a lot of 'core' theming that's unlikely to change much and might worth separating out (e.g. spacing, font sizes etc.). Better yet splitting the theme across layout, typography, colors etc. might be useful. In either case I'd probably recommend we come up with a system that allows one theme to inherit from another (e.g. the core unbranded), and making changes on top instead of writing from scratch.
This again I'd recommend as a future task, as what's there currently is still really nice to have and a good base to work from.
- Given that this PR touches pretty much every component in the codebase and we have no form of automated visual regression testing, I'd strongly suggest/request we have a deep manual review like you've already started @davehakkens - aware that really we want to test everything (from main pages, hovering stuff, clicking buttons, seeing popups while uploading a howto etc.).
Otherwise from the code side I'm happy to approve, overall the changes are a really nice addition to have and unlock a lot of exciting things for the future!!! Thanks again!
Looking forward to getting this merged once we get design sign-off from @davehakkens and can get the tests to pass
|
As for the tests - I've just checked out and tried running locally. There's quite a lot of issues with some of the props inheritance (trying to use I think this is gonna take a deeper dive to untangle than I have time for right now, so I'll wait to hear back from @thisislawatts in case any of this is resolved in #1300 and if not we can perhaps troubleshoot on slack if useful |
|
Closing in favour of #1300, leaving this branch hanging around for a little longer for reference. |
PR Checklist
masterbranch mergedPR Type
Description
Eventually the community-platform service will be installed as multiple instances to help grow communities around fixing fashion and sustainable living. This PR represents a step towards that goal by moving the styling out of individual components to a higher level abstraction represented by the type
PlatformTheme.DevSiteHeadercomponent, which is mainly for demo purposes to demonstrate how the site can be themed for different instances.For the reviewer:
Screenshots/Videos