[BOOKINGSG-9308][IT] Migrate uneditable section component#1179
[BOOKINGSG-9308][IT] Migrate uneditable section component#1179iant-gds wants to merge 7 commits into
Conversation
iant-gds
requested review from
ghazwan-gt,
qroll,
ryan-nguyen-t,
zhaoyanxzy and
ziggyzet
There was a problem hiding this comment.
Lets add a border to indicate the custom sections
There was a problem hiding this comment.
It seems this is meant to be deleted and we're using the Partial-custom one
There was a problem hiding this comment.
Lets not include loading into the cases as the animation makes it unstable
There was a problem hiding this comment.
we can still include loading, should be alright since this is a css animation, and playwright pauses it accordingly. but the timer manipulation needs to be consistent. link-list did something similar, can reference that
| grid-column: 1 / -1; | ||
| } | ||
|
|
||
| overflow-wrap: break-word; |
There was a problem hiding this comment.
Lets move this to the top alongside other properties before any selector
| export const fullWidthWrapperBackground = css` | ||
| background: ${Colour["bg-strong"]}; | ||
| `; |
There was a problem hiding this comment.
Both full width and regular have the same background, let's just use one
There was a problem hiding this comment.
hmm this doesn't look right, it's supposed to switch to 1 column view on mobile
There was a problem hiding this comment.
can we include a mobile screenshot of this?
There was a problem hiding this comment.
we can still include loading, should be alright since this is a css animation, and playwright pauses it accordingly. but the timer manipulation needs to be consistent. link-list did something similar, can reference that
There was a problem hiding this comment.
ehhhh we don't probably don't need ALL masked variants since a lot of this is programmatic, and doesn't have a visual difference
just check if they are covered in the unit tests, if not, add the scenarios there. remove from the e2e test
| &[data-width="half"].containerFullWidth { | ||
| grid-column: auto / span 1; | ||
| } |
There was a problem hiding this comment.
hmm why are the classes still nested? and the class name will change in the final build so we should not be hardcoding the class name like this
| `; | ||
|
|
||
| export const fullWidthWrapper = css` | ||
| background: transparent; |
There was a problem hiding this comment.
divs are transparent by default so this class looks unnecessary
| `; | ||
|
|
||
| export const wrapper = css` | ||
| background: transparent; |
There was a problem hiding this comment.
as above. unnecessary property
3 participants
Checklist
classNameis chained correctly withclsx- [ ] User style prop is set as CSS variable- [ ] Updated Storybook documentation- [ ] Listed breaking changes