Feat/fixSLStyling #1440
Feat/fixSLStyling #1440
Conversation
kishore03109
requested review from
seaerchin and
a team
and removed request for
seaerchin
src/contexts/SiteLaunchContext.tsx
Outdated
| @@ -79,7 +83,7 @@ export const SiteLaunchProvider = ({ | |||
| }) | |||
|
|
|||
| const { data: siteLaunchDto } = useGetSiteLaunchStatus(siteName) | |||
|
|
|||
| console.log({ siteLaunchDto }) | |||
| handleIncrementStepNumber: () => void | ||
| handleDecrementStepNumber: () => void |
There was a problem hiding this comment.
we need to take care to prevent props explosion here. this is not an issue for now because idt we will have active work going on for site launch, but we want to avoid the case where we just put everything here and ignore proper component structure, which will impact maintainability in the future.
tl;dr: is ok but we should avoid adding stuff here already.
There was a problem hiding this comment.
can, but for future ref may I trouble you to explain how I could make this more maintainable + allow storybook to be versatile enough to be able to click thr the checkboxes? I am abit unsure of how I could have made this PR better in the future
There was a problem hiding this comment.
without pair programming/actually working on this portion of the codebase, it's bit difficult to actually find something that works well. 1 sign that you could use to signal for help could be that you find yourself needing the context for everything, which shouldn't be the case.
1 thing that you can try is to see which parts of the state your child components truly need and refactoring so that you pass that state to your children. i've done htis here because the two nested components were p disjoint in their props but only really needed the selectedOption
| whiteSpace="nowrap" | ||
| overflow="hidden" | ||
| textOverflow="ellipsis" |
There was a problem hiding this comment.
somehow when I use `noOfLines`, the `...` (ellipsis) effect disappears, do you know why?
There was a problem hiding this comment.
have to check the styling - the Text hasn't overflowed its bounding box yet i think
There was a problem hiding this comment.
cos from the docs here, it should display ellipsis (and in prior experience, it has)
|
This pull request has been stale for more than 30 days! Could someone please take a look at it @isomerpages/iso-engineers |
1 similar comment
|
This pull request has been stale for more than 30 days! Could someone please take a look at it @isomerpages/iso-engineers |
|
This pull request has been stale for more than 30 days! Could someone please take a look at it @isomerpages/iso-engineers |
2 similar comments
|
This pull request has been stale for more than 30 days! Could someone please take a look at it @isomerpages/iso-engineers |
|
This pull request has been stale for more than 30 days! Could someone please take a look at it @isomerpages/iso-engineers |
Problem
Currently storybooks cannot toggle the steps, design found it harder to review + write a guide for SL
This also fixes trivial copy issues.
Solution
Change API so the
increaseStepNumberand thedecreaseStepNumberboth come from the context directly, similar to theincreasePageNumber+decreaseStepNumber.For Verbosity, I have also added 4 screens for design to review + increase possibilities of catching regression when other devs develop SL in the future.
Breaking Changes
Before & After Screenshots
AFTER:
Storybook check boxes should work
New Domain:
https://github.com/isomerpages/isomercms-frontend/assets/42832651/981f7417-4c6f-48d8-a34c-61d2609f38c9
Old Domain:
https://github.com/isomerpages/isomercms-frontend/assets/42832651/6cee2d80-3afe-4609-b2b3-c01844e59661
4 added storybooks:
A:
new-domain-non-www-dns-recordsB:
new-domain-www-dns-recordsC:
old-domain-non-www-dns-recordsD:
old-domain-www-dns-recordsNow copied got tooltip:
Tests
None, mostly to sped up reviewal + trivial copy changes