Add container level context fairground containers #12710
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +78 B (+0.01%) Total Size: 928 kB ℹ️ View Unchanged
|
abeddow91
force-pushed
the
ab/container-levels
branch
from
1a130b9
to
a3b9ef8
Compare
abeddow91
commented
Oct 31, 2024
| @@ -45,6 +60,14 @@ const findCollectionSuitableForFrontBranding = ( | |||
| return index; | |||
| }; | |||
|
|
|||
| const getContainerLevel = ( | |||
| levels?: FEContainerPalette[], | |||
There was a problem hiding this comment.
Currently, the level is included in the config meta from frontend. This is largely only used for container palette which is why the type here might seem strange.
There are 3 options here:
- Leave as is with the comment
- Rename FEContainerPalette to be more generic and better represent the metadata.
- Separate out the container level tags from the container palette tags on frontend model. There is a spike here for this work Pass collection level on the Frontend config model frontend#27574
There was a problem hiding this comment.
Thanks for writing up these options! I think option 1 is fine for now with a view that we could improve to do 2 and/or 3 as a follow up improvement
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
github-actions
bot
removed
the
run_chromatic
cemms1
approved these changes
Oct 31, 2024
Comment on lines
+23
to
+31
| const FAIRGROUND_CONTAINERS = [ | ||
| 'scrollable/feature', | ||
| 'static/feature/2', | ||
| 'flexible/special', | ||
| 'flexible/general', | ||
| 'scrollable/small', | ||
| 'scrollable/medium', | ||
| 'static/medium/4', | ||
| ]; |
There was a problem hiding this comment.
agree ideally these shouldn't need to be here - this should come from upstream data
| @@ -45,6 +60,14 @@ const findCollectionSuitableForFrontBranding = ( | |||
| return index; | |||
| }; | |||
|
|
|||
| const getContainerLevel = ( | |||
| levels?: FEContainerPalette[], | |||
There was a problem hiding this comment.
Thanks for writing up these options! I think option 1 is fine for now with a view that we could improve to do 2 and/or 3 as a follow up improvement
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this change?
Introduces container level context to DCR to allow fairground containers to be styled appropriately.
It also passes this through to the container title and applies styling there.
There is a secondary piece of work to bump Frontend to the latest verison of facia-scala-client in order for this data to pass through to DCR.
The container level tag lives in the metadata supplied by frontend. Currently, this field is used by the platform mainly for container palette tags and the types reflect this. This might be confusing for future developers. I have listed some options in this PR for discussion.
Why?
This is a requirement for "fairground" containers (ie new containers introduced as part of the fairground redesign project). These containers are:
Screenshots