Kuwait Theme: Component styling Updates #2521
Conversation
…pp-builder into style/update-comp-styles
|
@esmeetewinkel After these styling changes are reviewed and merged, you should have more variants for the |
There was a problem hiding this comment.
Thanks, @FaithDaka, it's great to see the visual changes, and nice to have the screenshots clearly displayed in the PR description for comparison.
I've left few comments inline, we can discuss in our meeting shortly
| @@ -8,7 +8,9 @@ interface ITextBubbleParams { | |||
| /** TEMPLATE PARAMETER: "speaker_position". The position of the speaker image and speech bubble tail */ | |||
| speakerPosition: "left" | "right"; | |||
| /** TEMPLATE PARAMETER: "variant" */ | |||
| variant: "gray" | "primary" | "secondary" | "no-border"; | |||
| variant: "gray" | "primary" | "secondary" | "no-border" | "orange" | "yellow"; | |||
There was a problem hiding this comment.
I think rather than "orange" and "yellow", these should be given general names so that the specific colours can be different for a different theme.
I would suggest maybe accent-1 and accent-2? We don't necessarily need to actually expose "accent" colours to the theme generation yet, but naming these variants in a way that is compatible with exposing the colours to the themes should prevent issues in the future.
There was a problem hiding this comment.
Thanks for highlighting this @jfmcquade. The variant names have been updated within the code and component sheet
There was a problem hiding this comment.
@jfmcquade - I'm not sure if we want to be introducing an accent variable when using primary and secondary (we previously agreed adding tertiary and quaternary was super confusing and removed from legacy code, so I don't see how accent-1 and accent-2 is much different). If however the accent is specific to the component and not the theme, then again I don't see the benefit in calling it something other than what it is, i.e. the orange or yellow variant.
If we're at the point of just wanting a fixed, hardcoded colour would it not make more sense to expose a parameter_list variable to manually override? Although I guess this brings up a follow-up question of whether this should be a CSS colour or another named colour from a list like https://tailwindcss.com/docs/customizing-colors
There was a problem hiding this comment.
@chrismclarke Yeh I take your point, it may not make sense to introduce another theme variable here. I think I was anticipating that we'll soon have another deployment making use of this component in various colours (@esmeetewinkel I believe there's one such deployment on the horizon?), and in that case we're likely to want the colours to be different to the ones used here. According to our incomplete style/variant system (see #1971), I think it's implicit that a style might specify a specific property value (like a colour), whereas a variant should have a more general name that can be interpreted differently by different themes. In that case, the current colour variants might make more sense as "styles" in that system, although we haven't implemented styles in this way yet – also, the fact that the variant changes both the background and border colour does mean it might be suited to have those style changes "wrapped up" in a named variant.
@FaithDaka has highlighted that from a design perspective, we are going to want to offer more than two theme level colour variables at some point (the plh_kids_kw designs uses a palette in line with the material design colour system, with at least 3 or 4 colours used in various shades). The yellow colour used in the text-bubble component variant is already being used in a different shade in the select-text component styling changes included in this PR. Currently this is hardcoded, and I was thinking that the process of adapting our existing theme system would occur at the same time as incorporating (a subset of) the plh_kids_kw style changes into our other themes, i.e. after the plh_kids_kw deployment has been released.
In any case, as we're likely not actually implementing the new theme-level colour variables at this point, I'm happy to revert these to explicit colour names. We'll likely need to give it some more thought about how to incorporate additional theme colours beyond the current primary/secondary. But I think this is something we will need to support at some point, and it could be good to get your perspective then, @FaithDaka
There was a problem hiding this comment.
Hmm, ok in that case I might backtrack a little here. So what I understand is that we now have multiple components that are likely to want to use 3+ colour variants all within the same app (e.g. multiple speaker text bubbles).
I think what makes the most sense here is to expose a parameter_list option alongside CSS variable. If we have a in the component styles
--text-bubble-color-1: var(--color-primary);
--text-bubble-color-2: var(--color-secondary);
--text-bubble-color-3: var(--color-primary);
--text-bubble-color-4: var(--color-secondary)And then any deployment can simply provide overrides as needed in the theme styles. E.g.
--text-bubble-color-3: orange;
--text-bubble-color-4: yellow;A little extra CSS magic would be needed to handle either making the border a darker shade or background a lighter shade depending on how we want to interpret the provided colour (although still think easier that than exposing two variables for each colour, e.g. --text-bubble-color-bg-1 and --text-bubble-color-border-1)
Using the parameter list we can expose (if not already there) something like speaker_number: 1 used to handle position and colour of multiple speakers.
What do you think @jfmcquade ?
There was a problem hiding this comment.
(oh, and will need to change the component style demo slightly so that the component style inherits from the global style and not override as my example would do)
There was a problem hiding this comment.
Using the parameter list we can expose (if not already there) something like speaker_number: 1 used to handle position and colour of multiple speakers.
FYI I'm handling the position, colour, icon and name of the speaker in a combination of a data list and template:
There was a problem hiding this comment.
I think what makes the most sense here is to expose a parameter_list option alongside CSS variable. If we have a in the component styles
I think this is a clever solution. However after trying to implement it I'm not sure it quite works: if we replace the position and variant parameters with a single "speaker_number" parameter, then we lose the ability to customise the variant and position independently (for example authoring a primary coloured bubble on the right hand side, or a grey bubble anywhere), but if we keep the variant and position properties but add an additional speaker_number property, then this latter property is redundant apart from being a more efficient way to author combinations of variant and position.
As I see it, the problem we're trying to solve is to expose an authoring option to set the colour without naming a specific colour (e.g. "orange") or using a colour name that suggests a theme colour that doesn't actually exist (e.g. "accent-1").
I've implemented a sort of compromise with 71bd2d3: the third and fourth colour variants are still authored using an explicitly named variant, but these variants are named "speaker-3" and "speaker-4", to avoid naming a colour or theme-colour. The colours associated with those variants are exposed as theme-level variables, with fallbacks as the theme primary and secondary colours, as you suggested.
A little extra CSS magic would be needed to handle either making the border a darker shade or background a lighter shade depending on how we want to interpret the provided colour (although still think easier that than exposing two variables for each colour, e.g. --text-bubble-color-bg-1 and --text-bubble-color-border-1)
I've opted to use two explicit variables for each in order to keep it fully customisable (and, as we're limited to runtime CSS functions to manipulate the values of CSS var(--...)s, I wasn't too pleased with the available options for darkening/lightening the colour dynamically)
Let me know what you think of the changes in 71bd2d3, @chrismclarke and @FaithDaka
| @@ -8,7 +8,9 @@ interface ITextBubbleParams { | |||
| /** TEMPLATE PARAMETER: "speaker_position". The position of the speaker image and speech bubble tail */ | |||
| speakerPosition: "left" | "right"; | |||
| /** TEMPLATE PARAMETER: "variant" */ | |||
| variant: "gray" | "primary" | "secondary" | "no-border"; | |||
| variant: "gray" | "primary" | "secondary" | "no-border" | "orange" | "yellow"; | |||
| /** TEMPLATE PARAMETER: "speaker_name". The name of the speaker */ | |||
There was a problem hiding this comment.
Good to have this comment in line with our documentation convention. It would also be good to explicitly list the new parameter in the PR description, that way @esmeetewinkel can functionally test the new functionality without needing to engage with the code, plus the PR can act as a form of documentation for future reference.
The same goes for the new variants: it would be good to explicitly lit them in the PR description
There was a problem hiding this comment.
The PR description has been updated to list the new parameters @jfmcquade
| [attr.data-rowname]="_row.name" | ||
| ></plh-template-component> | ||
| <div> | ||
| @if (params.speakerName) { |
There was a problem hiding this comment.
In order to preserve consistency with non-Kuwait themes, we might need to make this element hidden on every other theme and only display it when the plh_kids_kw is active. Otherwise, we should style it so that it looks good on the other themes too. It would also be good to add an example to the comp_text_bubble template so that we can see how it displays
|
Visit the preview URL for this PR (updated for commit ce92cf4): https://plh-teens-app1--pr2521-style-update-comp-st-h60inx5m.web.app (expires Thu, 05 Dec 2024 15:56:47 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: e4c0bab6b08dd290fbf002fd6e07987fa4b5fce1 |
…pp-builder into style/update-comp-styles
| @@ -8,7 +8,9 @@ interface ITextBubbleParams { | |||
| /** TEMPLATE PARAMETER: "speaker_position". The position of the speaker image and speech bubble tail */ | |||
| speakerPosition: "left" | "right"; | |||
| /** TEMPLATE PARAMETER: "variant" */ | |||
| variant: "gray" | "primary" | "secondary" | "no-border"; | |||
| variant: "gray" | "primary" | "secondary" | "no-border" | "speaker-3" | "speaker-4"; | |||
| /** TEMPLATE PARAMETER: "avatar_name". The name of the speaker */ | |||
There was a problem hiding this comment.
nit(non-blocking): I assume this comment can also be updated now to speaker_name.
Also not sure if we're really using a name or not here. Looking at the variants above I'm guessing we're more using a number to identify which speaker within a list of potential speakers that have different styling. If so might make more sense to use speaker_number. But I don't feel super strongly either way
| .container[data-position="right"] { | ||
| .speaker-name { | ||
| text-align: right; | ||
| right: 80px; |
There was a problem hiding this comment.
nit(non-blocking): Just to quickly check, will this behave as expected when using a right-to-left language? Not sure in this case whether the text or avatar would be on the left/right (might be good to double-check)
| .speaker-name { | ||
| position: absolute; | ||
| bottom: -55px; | ||
| left: 78px; |
There was a problem hiding this comment.
nit(non-blocking): I always start to get a little anxious when it comes to absolute-positioned elements. They tend to break quite quickly when making changes elsewhere, and often are harder to identify in the dom (appear out of order). Assuming this is the text that goes alongside an avatar/image then it might be worth trying to just wrap both in a flex box.
If there is a strong preference to keep absolute positioning, I'd suggest using numbers which are a multiple of 16 or 8, as more likely than not these will align more closely with standard font sizes (16px). E.g. I'd set bottom as -56px instead of -55, and left as 80px instead of 78 (unless wanting a more off-center appearance)
There was a problem hiding this comment.
Overall happy with the way this is shaping up, I'll leave it to @jfmcquade to resolve any minor comments with you @FaithDaka
…ternational/open-app-builder into style/update-comp-styles
…pp-builder into style/update-comp-styles
PR Checklist
Description
plh_kids_kwtheme: Changes to base components styling (text, input box, speech-bubbles, select-text, accordion)Author Notes
Text BubbleVariantsvariantparameter_list:accent-1andaccent-2.For the Kuwait theme the
accent-1variant corresponds to the orange text bubble while theaccent-2variant corresponds to the yellow text bubble, as used in the comics. Refer to screenshot in column 1 for visuals.primaryvariant is the main variant used by the facilitator within the module articles.secondaryvariant is used on the feelings check-in page and has a gold outline.grayvariant is used in the quiz response when a user selects an answer to the question asked. It has a gray outline.Text BubbleSpeaker Namespeaker_nameType:stringExample - speaker_name: Dalal;
Git Issues
Closes #2533
Screenshots