Feature/optional header #989
Conversation
There was a problem hiding this comment.
I'm a bit concerned about both the ThemeConfig and the DefaultPage being somewhere in between something generic and something Experiment(Collection) oriented, though I'm not sure what the solution should be. Secondly, I feel like the frontend hasn't been finished properly: The experiment tiles seem to be vertically stacked and the other theme properties haven't been implemented. As for the latter, I think you could just copy + paste the font loader & background image configuration from the Experiment or perhaps even use this opportunity to abstract the font loader & background image to one single page/layout.
backend/experiment/serializers.py
Outdated
| @@ -34,6 +34,7 @@ def serialize_experiment_collection( | |||
| 'description': experiment_collection.description, | |||
| 'consent': consent, | |||
| 'about_content': about_content, | |||
| 'theme_id': experiment_collection.theme_config.pk if experiment_collection.theme_config else None | |||
There was a problem hiding this comment.
I was wondering why you are sending the theme's id here instead of serialising the theme like we are doing in the experiment. Now you have to first fetch the experiment collection and then do a second http request for fetching the theme. Won't that result in some flashy/glitchy behaviour as you first seen the themeless version and then suddenly the theme might pop up after it's done fetching?
There was a problem hiding this comment.
I did this based on earlier feedback in which you seemed to be concerned about mixing different models in one serialization.
backend/theme/serializers.py
Outdated
| def serialize_footer(footer: FooterConfig) -> dict: | ||
| return { | ||
| 'disclaimer': footer.disclaimer, | ||
| 'logos': [ | ||
| join(settings.MEDIA_URL, str(logo.file)) for logo in footer.logos.all() | ||
| ], | ||
| 'privacy': footer.privacy | ||
| } | ||
|
|
||
|
|
||
| def serialize_header(header: HeaderConfig) -> dict: | ||
| return { | ||
| 'next_experiment_button_text': _('Next experiment'), | ||
| 'about_button_text': _('About us'), | ||
| 'show_score': header.show_score | ||
| } |
There was a problem hiding this comment.
Now that we have experiment collection specific properties in the theme, does that mean themes are now officially used for experiment collections and we will phase out the theme for specific experiments, or what do you think about this?
There was a problem hiding this comment.
Indeed, with #928 as a proposed course of action, I think the link of Experiment to ThemeConfig can be removed eventually.
| <DefaultPage | ||
| collectionSlug={experimentCollection.slug} | ||
| nextExperimentSlug={nextExperimentSlug} | ||
| > |
There was a problem hiding this comment.
Not sure if it's only on my machine but the tiles are showing as a vertical stack
There was a problem hiding this comment.
I think it has to do with .aha__page .container having a max-width: 600px !important; in Page.scss
|
|
||
| // DefaultPage is a Page with an AppBar and a width-restricted container for content | ||
| const DefaultPage = ({ className, title, logoClickConfirm, children }) => { | ||
| const DefaultPage = ({ className, title, logoClickConfirm, collectionSlug='', nextExperimentSlug='', children }) => { |
There was a problem hiding this comment.
Is the/a DefaultPage is a default page in the sense of a generic wrapper whose purpose is to only be a layout for all/some pages, should it really contain properties related to experiments or collections? It seems to become more like a ExperimentCollectionPage now.
There was a problem hiding this comment.
Perhaps I should indeed make a new component.
| const DefaultPage = ({ className, title, logoClickConfirm, children }) => { | ||
| const DefaultPage = ({ className, title, logoClickConfirm, collectionSlug='', nextExperimentSlug='', children }) => { | ||
|
|
||
| const theme = useBoundStore((state) => state.theme); |
There was a problem hiding this comment.
Perhaps it was a conscious decision, but the fonts and background image are not shown like they are on the experiment page:
// Experiment.jsx
<FontLoader fontUrl={theme?.heading_font_url} fontType="heading" />
<FontLoader fontUrl={theme?.body_font_url} fontType="body" />// Page.jsx
const theme = useBoundStore((state) => state.theme);
const backgroundImageUrl = theme?.background_image || '/public/images/background.jpg';There was a problem hiding this comment.
This should indeed still be adjusted.
| if (experimentCollection?.theme_id) { | ||
| getTheme(experimentCollection.theme_id).then( (theme: ITheme) => | ||
| setTheme(theme) | ||
| ) | ||
| } |
There was a problem hiding this comment.
As mentioned before, I think it might be better to directly populate the theme property on the experiment collection so you don't have to do two http requests in a row.
Co-authored-by: Drikus Roor <drikusroor@gmail.com>
This reverts commit 76e1863.
Co-authored-by: Drikus Roor <drikusroor@gmail.com>
Co-authored-by: Drikus Roor <drikusroor@gmail.com>
…m-Music-Lab/MUSCLE into feature/optional-header
Close #941 by adding a configurable header to the theme.
ExperimentCollectionDashboardwill display a<Header>component if a theme with header is set.Something that's now inconsistent with other views, but perhaps more consistent with json guidelines, is that the dicts sent to the frontend for
ExperimentCollectionobjects will be in camel case, rather than snake case.