-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: redesign InfoCards component and update schema #414
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
92897bd
to
c50c92a
Compare
}: ControlProps) { | ||
schema, | ||
}: ControlProps): JSX.Element { | ||
if (schema.const !== undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's schema.const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
schema.const
is a JSON Schema construct where the particular property takes in a specific literal (can be string/number/boolean). In this case, if the const
is defined (say it's true) then it is not meaningful to show the toggle, since the
false` case is invalid.
@@ -84,6 +84,7 @@ export const DEFAULT_BLOCKS: Record< | |||
type: "infocards", | |||
title: "This is an optional title of the Infocards component", | |||
subtitle: "This is an optional subtitle for the Infocards component", | |||
isCardsWithImages: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think there can be other states for cards? if so we can use an enum like "WithImages, TextOnly" etc. and is extensible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good point, this was the original based on designs where there was a toggle button but this has changed to use a dropdown to select the variants. Adjusted in 34dcbfd!
c0f9b58
to
1d97a4f
Compare
1d97a4f
to
08ffbbc
Compare
910cbe3
to
7fa4c7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run formatted on generated stuff pls :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebase removed le
34dcbfd
to
e98e00c
Compare
Updated with styling tweaks. Please re-review! |
"imageUrl" | "imageAlt" | "url" | ||
>): JSX.Element => ( | ||
<div | ||
className={compoundStyles.cardImageContainer({ isClickableCard: !!url })} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcshzj I had to pass icClickableCard
here as well because the hover effect applies to both InfoCardImage
(hover on image) AND cardTitle
(color change, arrow moves). Previously group-hover
wasn't working on the InfoCardImage
. Hope this is not weird in terms of practice though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup this works!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm if design passes design
...es/editing-experience/components/form-builder/renderers/controls/JsonFormsBooleanControl.tsx
Outdated
Show resolved
Hide resolved
@@ -11,5 +11,6 @@ | |||
"devDependencies": { | |||
"@isomer/eslint-config": "*", | |||
"@isomer/prettier-config": "*" | |||
} | |||
}, | |||
"prettier": "@isomer/prettier-config" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need run formatting again since all tooling affected hehehe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay for some reason it didn't give me anything the first time round, fixed in d2a6419.
Problem
InfoCards component has drifted from the Figma designs.
Solution
Breaking Changes
Improvements:
type
properties.