Kuwait Theme: Settings page styling #2598
Conversation
…pp-builder into feature-kw-settings-page
There was a problem hiding this comment.
We're trying to move away from setting styles via the style parameter – instead, named styles like list-title and page-title should be variants. This makes it easier to track what variant(/style) options are available for a given component, and also allows style to be used for applying global styles that make sense on multiple components (we haven't implemented this yet, but the plan is outlined in #1971 if you're interested 😅).
I would also suggest that page-title and list-title be variants of the dedicated title component (src/app/shared/components/template/components/title/title.component.ts), unless there is good reason to use the text component.
…pp-builder into feature-kw-settings-page
|
Changes made @jfmcquade |
…pp-builder into feature-kw-settings-page
There was a problem hiding this comment.
Thanks, @FaithDaka, I've added a few comments inline, plus some requests below:
For the button component, there are some issues with achieving your design by using the round button component inside a child row of the button component:
- The spacing doesn't look quite right
- The round button component is a different click target to its parent button
I think it would be better to pass in the secondary icon as an additional parameter to the button component, e.g. an icon_secondary param, and then pass this icon to the end slot of the ion-button inside src/app/shared/components/template/components/button/button.component.html, something like this:
Changes made @jfmcquade Though I failed to understand how to use the title template for review.
You're right, it looks like the comp_title template wasn't fit for purpose. I've updated it now. You can add rows to demo the new variants included in this PR when you have finalised the names
| @@ -31,6 +31,8 @@ interface IButtonParams { | |||
| icon: string; | |||
| /** TEMPLATE PARAMETER: "image_asset". The path to an image asset */ | |||
| image: string; | |||
| /** TEMPLATE PARAMETER: "icon_position". The alignment of the icon on the button */ | |||
| iconPosition: "left" | "right"; | |||
There was a problem hiding this comment.
This param can be removed as I don't believe it's used
| @@ -13,7 +13,7 @@ interface ITitleParams { | |||
| /** TEMPLATE PARAMETER: "style". */ | |||
| style: string | null; | |||
| /** TEMPLATE PARAMETER: "variant". */ | |||
| variant: "" | "header"; | |||
| variant: "" | "header" | "list-title" | "page-title"; | |||
There was a problem hiding this comment.
Now that these are variants of the title component itself, they should be renamed. I think the wording could be more descriptive/accurate/general as well, how about:
banner_pageandbanner_sectionbannerandbanner_sub
Or something along those lines
…ternational/open-app-builder into feature-kw-settings-page
…pp-builder into feature-kw-settings-page
…pp-builder into feature-kw-settings-page
|
Changes made @jfmcquade |
There was a problem hiding this comment.
As outlined above, I don't think that using the round button inside the button component is a good solution. I've updated the button component to take a secondary icon instead, see a5f24f3, and updated the component demo template.
I will update the PR description accordingly and request a functional review
PR Checklist
Description
Includes styling for components on the Settings page. Button, Title
Author Notes
Use the button params:
style: no-backgroundto achieve the button look for the options listUse the title component with params:
variant: section_bannerto achieve the text look for the options titleUse the title component with params:
variant: page_bannerto achieve the text look for the settings page titleGit Issues
Closes #2582