[doc] Theme generator review step added#2420
[doc] Theme generator review step added#2420Jialecl wants to merge 4 commits intotheme-generatorfrom
Conversation
| > | ||
| <DxcFlex direction="column" gap="var(--spacing-gap-ml)"> | ||
| <DxcTypography fontSize="var(--typography-title-l)" fontWeight="var(--typography-title-bold)"> | ||
| Branding Assets |
There was a problem hiding this comment.
Assets should be in lower case
| const ReviewBrandingAssets = ({ logos }: { logos: Logos }) => { | ||
| return ( | ||
| <DxcGrid templateColumns={["repeat(4, 1fr)"]} templateRows={["156px"]} gap="var(--spacing-gap-m)"> | ||
| <BrandingAsset label="Main Logo" logo={logos.mainLogo[0]?.preview || ""} /> |
There was a problem hiding this comment.
Logo should be in lowercase, like others
|
|
||
| const ReviewBrandingAssets = ({ logos }: { logos: Logos }) => { | ||
| return ( | ||
| <DxcGrid templateColumns={["repeat(4, 1fr)"]} templateRows={["156px"]} gap="var(--spacing-gap-m)"> |
There was a problem hiding this comment.
Where do we get 156px?
There was a problem hiding this comment.
I set the height to the wrong element. The title + logo preview was 156px in design, but it was the image preview the one with a set height of 130px in the design. I have changed the element with the set height.
|
|
||
| const ReviewBrandingAssets = ({ logos }: { logos: Logos }) => { | ||
| return ( | ||
| <DxcGrid templateColumns={["repeat(4, 1fr)"]} templateRows={["156px"]} gap="var(--spacing-gap-m)"> |
There was a problem hiding this comment.
Gap should be 16px instead of 12px
| <DxcContainer width="250px" height="100%"> | ||
| <DxcFlex direction="column" gap="var(--spacing-gap-xs)" fullHeight> | ||
| <DxcTypography fontWeight="var(--typography-label-semibold)">{label}</DxcTypography> | ||
| <DxcContainer |
There was a problem hiding this comment.
Should we show anything if there is no logo selected?
There was a problem hiding this comment.
I did change this to show empty grey containers. There are some solutions that we can argue about:
- having default logos and use those by default
- leaving empty grey placeholders like now
- removing the Branding assets section in the review step if there are no logos and showing only those that do have a logo assigned
| return ( | ||
| <DxcContainer height="100%" overflow="auto" borderRadius="var(--border-radius-m)"> | ||
| <DxcTypography as="pre" fontSize="var(--typography-body-xs)" whiteSpace="pre"> | ||
| {themeJson} |
There was a problem hiding this comment.
This tokens list should also include logos, does it?
There was a problem hiding this comment.
Fixed. Moved the json string creation to ReviewDetails instead to avoid code repetition.
| }); | ||
| }; | ||
| return ( | ||
| <> |
There was a problem hiding this comment.
Pleas remove this fragment
There was a problem hiding this comment.
The component is returning two DxcContainers without this fragment we would receive an error.
| /> | ||
| </DxcFlex> | ||
| <DxcGrid templateColumns={["2fr", "1fr"]} templateRows={["368px"]} gap="var(--spacing-gap-m)"> | ||
| <DxcFlex justifyContent="center"> |
There was a problem hiding this comment.
Where do we get 368px?
There was a problem hiding this comment.
It is the height applied in the design.
| {logo && ( | ||
| <DxcFlex alignItems="center" justifyContent="center" fullHeight> | ||
| <DxcImage width="100%" height="100%" objectFit="contain" src={logo} alt={label} /> | ||
| </DxcFlex> |
There was a problem hiding this comment.
There is less padding space than in the design:
Design:
There was a problem hiding this comment.
Right now based on the design the padding that we would apply is not correct:
The image oversteps the padding.
| warning: CssColor; | ||
| }; | ||
|
|
||
| export type ColorGroups = { |
There was a problem hiding this comment.
Should it be TokenGroups intead of ColorGroups?
| ); | ||
| }; | ||
|
|
||
| export default ReviewTokensGrid; |
There was a problem hiding this comment.
{group} should be capitalized, as it is in the design
| const ReviewTokensList = ({ generatedTokens }: { generatedTokens: Tokens }) => { | ||
| const themeJson = useMemo(() => JSON.stringify(generatedTokens, null, 2), [generatedTokens]); | ||
| return ( | ||
| <DxcContainer height="100%" overflow="auto" borderRadius="var(--border-radius-m)"> |
There was a problem hiding this comment.
The scrollbar of this container is not the same as in the design, and is not positioned in the same spot.
There was a problem hiding this comment.
This topic has been discussed with design already.
| const ReviewBrandingAssets = ({ logos }: { logos: Logos }) => { | ||
| return ( | ||
| <DxcGrid templateColumns={["repeat(4, 1fr)"]} templateRows={["156px"]} gap="var(--spacing-gap-m)"> | ||
| <BrandingAsset label="Main Logo" logo={logos.mainLogo[0]?.preview || ""} /> |
There was a problem hiding this comment.
Instead of rendering the same component 4 times, we can define an array with the label and the logo key(mainLogo, footerLogo...).
Then iterate over that array and render .
| import { Logos } from "screens/theme-generator/types"; | ||
|
|
||
| const BrandingAsset = ({ label, logo }: { label: string; logo: string }) => { | ||
| return ( |
There was a problem hiding this comment.
Taking into account the comment about creating an array with a label and logo(l.30), we should check here if the logo exists or not.
| return ( | ||
| <DxcGrid templateColumns={["repeat(4, 1fr)"]} templateRows={["156px"]} gap="var(--spacing-gap-m)"> | ||
| <BrandingAsset label="Main Logo" logo={logos.mainLogo[0]?.preview || ""} /> | ||
| <BrandingAsset label="Footer logo" logo={logos.footerLogo[0]?.preview || ""} /> |
There was a problem hiding this comment.
I'm just curious as to why we're setting a fallback value (|| "") instead of typing the prop as a string | undefined.
There was a problem hiding this comment.
You are totally right. It was an oversight from my part
| const ReviewBrandingAssets = ({ logos }: { logos: Logos }) => { | ||
| return ( | ||
| <DxcGrid templateColumns={["repeat(4, 1fr)"]} templateRows={["156px"]} gap="var(--spacing-gap-m)"> | ||
| <BrandingAsset label="Main Logo" logo={logos.mainLogo[0]?.preview || ""} /> |
There was a problem hiding this comment.
For consistency, change 'Main Logo' to 'Main logo'.
| > | ||
| {SHADE_VALUES.map((value, index) => | ||
| index === 0 ? ( | ||
| <DxcGrid.Item column={"2/3"}> |
There was a problem hiding this comment.
Add 'key' to avoid warnings(do not use 'index' as its value).
| </DxcFlex> | ||
| {colors.map((color: string, index: number) => ( | ||
| <div | ||
| key={index} |
There was a problem hiding this comment.
Avoid using 'index' as key.
| }; | ||
| return ( | ||
| <> | ||
| <DxcContainer |
There was a problem hiding this comment.
Both containers (L.21 and L.50) share the exact same props. Consider extracting a shared component to avoid duplication.
| @@ -0,0 +1,70 @@ | |||
| import { DxcButton, DxcContainer, DxcFlex, DxcGrid, DxcTypography, useToast } from "@dxc-technology/halstack-react"; | |||
| import { Logos, Tokens } from "../types"; | |||
There was a problem hiding this comment.
Import paths are inconsistent: ReviewTokensGrid and ReviewTokensList use absolute paths while ReviewBrandingAssets and copyToClipboard use relative paths. For consistency, consider unifying to one style.
|
|
||
| const ReviewDetails = ({ generatedTokens, logos }: { generatedTokens: Tokens; logos: Logos }) => { | ||
| const toast = useToast(); | ||
| const handleCopy = (value: string) => { |
There was a problem hiding this comment.
Just as a comment: I think the same copyToClipboard + Toast pattern is used in the first step. If it ends up being needed in more places, it would be worth extracting it into a 'useCopyToClipboard' hook to centralise the logic.
There was a problem hiding this comment.
I do like the idea I will work on it in the mean time I will first push a commit with the rest of the requested changes/feedback.
- Fixed and added some keys in maps - Fixed capitalization in some labels/titles - Avoided some redundancy creating components for repeated elements - Changed some types' names - Some other minor changes
4 participants
Checklist
(Check off all the items before submitting)
/libdirectory./websiteas needed.Purpose
Theme generator third step added.