fix(accordion): styles #1834
fix(accordion): styles #1834
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @kishore03109 and the rest of your teammates on |
kishore03109
force-pushed
the
03-14-fix_accordion_styles
branch
from
bf61867 to
1b5e0ba
Compare
| @@ -31,6 +31,7 @@ export const BlockWrapper = ({ | |||
| backgroundColor="#055AFF" | |||
| textColor="white" | |||
| p="0.25rem" | |||
| mt="-0.75rem" | |||
There was a problem hiding this comment.
why's this required? from the review, this seems like accordion should have a 0.75rem gap, not that the gap between elements should be reduced by 0.75rem
There was a problem hiding this comment.
hey this crit from design was outdated (look at the Se Hyun's comments)
There was a problem hiding this comment.
on why this is needed,
before:
after:
src/layouts/components/Editor/extensions/Details/IsomerDetailGroupView.tsx
Outdated
Show resolved
Hide resolved
| mr="-0.5rem" | ||
| mt="-1.5rem" |
There was a problem hiding this comment.
could we need to use negative margins to remove padding? wouldn't this cause accordion to be out of alignment with the other elements
There was a problem hiding this comment.
on why this is needed
before:
after:
note that this needs to be consistent with card grid:
There was a problem hiding this comment.
hmm i get that - but the odd thing is i couldn't find negative margins being used in card grid ._.
i'm ok with this for now ba but if there's time, please try to figure out why card grids doesn't require the negative margin
| borderTop="1px solid #d4d4d4" | ||
| borderBottom="1px solid #d4d4d4" |
There was a problem hiding this comment.
should we use colour tokens here
There was a problem hiding this comment.
sadly we dont have a token for this yet.
@sehyunidaaa may i get your help in this ya
There was a problem hiding this comment.
ahh this is cos i used the colors from the template. my bad!
can use divider/strong from OGPDS please thank you
| borderColor="base.divider.medium" | ||
| display="flex" | ||
| justifyContent="flex-end" | ||
| boxShadow="0px 8px 12px 0px rgba(187, 187, 187, 0.50)" |
There was a problem hiding this comment.
could we use a token for this?
boxShadow: `0 0 0 1px var(--chakra-colors-gray-400)`There was a problem hiding this comment.
@sehyunidaaa may i get token for this please?
There was a problem hiding this comment.
There was a problem hiding this comment.
hmmm can we use this shadow-sm from OGPDS?
https://github.com/opengovsg/design-tokens/blob/09eeb37eaca44f0cb56e64d992cdee75b34e7661/tokens.json#L1109C1-L1110C1 ? might look a bit different but not earth-shattering, it's a small shadow
but we've been using the same bespoke shadow for other bubble menus btw
kishore03109
force-pushed
the
03-14-fix_accordion_styles
branch
from
d6a2715 to
555656f
Compare
kishore03109
force-pushed
the
03-14-fix_accordion_styles
branch
from
555656f to
825766a
Compare
Problem
Design review for accordions, addressing changes here.
Solution
Fix styling issues