Add a feature flag to select the desired UI design #1814
Conversation
If code should not land without matching a certain style, then it should fail to be approved (by CI). A pre-commit hook isn't what we want, imo (and in my specific case, I don't even have node installed on my system at all - I run all frontend tests and code in a container when needed which is pretty simple and nicer, imo). What I've don in other projects is to run required formatting checks on the code in CI, and if there are any changes after running, fail. Are you OK with that instead? That forces people (like myself) to follow any requirements without assumptions about where or how it is run. Let me know when you've rebased (gh already changed the base branch when I landed mine) and I'll review :) |
I disagree but that's a personal opinion. Even if you write unit tests to ensure that whatever you are implementing works as expected you will need to check if everything works end-to-end (also I usually discover issues even if they are not related to the new code). Building the dashboard image is quite slow, if you are working in cosmetic changes (CSS) I would say that's not even an option. Once you have the setup ready, it's quite easy to run the dashboard locally and changes are reflected instantaneously.
Yes, that's fine as well.
will do 👍 |
|
@absoludity this should be ready to be reviewed now |
| @@ -70,6 +68,7 @@ class Header extends React.Component<IHeaderProps, IHeaderState> { | |||
| const reposPath = `/config/ns/${cluster.currentNamespace}/repos`; | |||
| return ( | |||
| <section className="gradient-135-brand type-color-reverse type-color-reverse-anchor-reset"> | |||
| <UISelector UI={featureFlags.ui} /> | |||
There was a problem hiding this comment.
It's not clear to me here - does this actually make a selector appear in the UI? I assume not since we've got a feature flag to select which UI... so what does this component do?
There was a problem hiding this comment.
Nope, I called this "selector" because the component is in charge of selecting the css file to load (but maybe another name is more suitable?). The selection is made automatically based on the configuration so the user just sees the configured UI. You will see an example in the next PR.
| @@ -0,0 +1,17 @@ | |||
| import * as React from "react"; | |||
|
|
|||
| const HEx = React.lazy(() => import("./HEx")); | |||
There was a problem hiding this comment.
I see it matches the filename, but was that a typo? (the 'E' rather than 'e')
There was a problem hiding this comment.
nope, the design system is called "HEx": https://design.bitnami.com/
Yes - of course, I usually do an IRL check once the changes are done, but it forces me to plan the change and write the failing test, fix, next test, fix, etc., and only when I think the required changes are done, build the image and verify it works as expected... obviously sometimes I then find issues and go back to reproduce with a new failing test and fix etc. Yes, I could still do this without having to build the image at the end which would be slightly faster.
Yes - it's very useful for cosmetic changes. I've not done (and don't often do) large style changes, but I'd definitely use it for that.
Yep, I'll go back and try again to see if I can get it working within a container and if not, install it locally (I'd just prefer to have things like this running in a container where possible). |
Description of the change
The goal of this PR is to conditionally load the required CSS for hex or for clarity based on a feature flag. Thanks to
React.SuspenseandReact.lazywe are able to dynamically load components. The goal of the CSSSelector component is to load either the HEx or Clarity components that loads the CSS file.Applicable issues
Additional info
This PR is based on #1813, there are ton of minor style changes due to the changes there (and master). @absoludity can you enable the pre-commit hook so those changes are attached to your PRs in the future?