hotfix/pin-5752-hide-signal-hub-eservices #1025
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
martinaCampoli
requested review from
ruggerocastagnola and
Carminepo2
as code owners
Carminepo2
requested changes
Dec 10, 2024
src/pages/ConsumerEServiceCatalogPage/components/CatalogCard.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
38
to
52
| const sxCardProps: SxProps = | ||
| disabled === true | ||
| ? { | ||
| height: '100%', | ||
| display: 'flex', | ||
| flexDirection: 'column', | ||
| minHeight: 410, | ||
| opacity: 0.5, | ||
| } | ||
| : { | ||
| height: '100%', | ||
| display: 'flex', | ||
| flexDirection: 'column', | ||
| minHeight: 410, | ||
| } |
There was a problem hiding this comment.
Suggested change
| const sxCardProps: SxProps = | |
| disabled === true | |
| ? { | |
| height: '100%', | |
| display: 'flex', | |
| flexDirection: 'column', | |
| minHeight: 410, | |
| opacity: 0.5, | |
| } | |
| : { | |
| height: '100%', | |
| display: 'flex', | |
| flexDirection: 'column', | |
| minHeight: 410, | |
| } | |
| const sxCardProps: SxProps = { | |
| height: '100%', | |
| display: 'flex', | |
| flexDirection: 'column', | |
| minHeight: 410, | |
| opacity: disabled ? 0.5 : 1, | |
| } |
There was a problem hiding this comment.
This could be simplified as such.
Plus, since it is used just once, I would put it "inside" the prop
Comment on lines
54
to
76
| const linkEservice = ( | ||
| <> | ||
| <Tooltip key="subscribe-tooltip" title={disabled ? t('subscribeTooltip') : ''} arrow> | ||
| <span> | ||
| <Link | ||
| as="button" | ||
| size="small" | ||
| variant="contained" | ||
| to="SUBSCRIBE_CATALOG_VIEW" | ||
| params={{ | ||
| eserviceId: eservice.id, | ||
| descriptorId: eservice.activeDescriptor?.id ?? '', | ||
| }} | ||
| onFocusVisible={handlePrefetch} | ||
| color="primary" | ||
| disabled={disabled} | ||
| > | ||
| {tCommon('actions.inspect')} | ||
| </Link> | ||
| </span> | ||
| </Tooltip> | ||
| </> | ||
| ) |
There was a problem hiding this comment.
Why did you extract it in a separate constant?
The React fragments and the key prop are not needed.
To make the Tooltip "optional" you need to pass the open prop and optionally pass false or undefined, you can see an example here /src/components/shared/StepActions.tsx.
You can also remove the ternary on the title prop
src/config/constants.ts
Outdated
Comment on lines
41
to
52
| export const SH_ESERVICES_ATT_TEMP = ['9b6993ee-60e3-4901-9a32-e6987d690ec4'] //signal hub eservices to hide | ||
| export const SH_ESERVICES_UAT_TEMP = [ | ||
| //signal hub eservices to hide | ||
| '7ab0a0fc-7d22-4007-b2f3-fddd68fe2f17', | ||
| 'e8c087eb-627b-4488-a9b7-65b70fd1301b', | ||
| '407edf51-23b5-462b-af6e-128bbaa4d9ff', | ||
| '3b0fbe47-2e2c-4d8b-9cff-b2381c92d003', | ||
| '260e45e1-9a61-49d6-8b6d-da0643da68ac', | ||
| 'a2b84a6e-34cf-44ca-85a4-de21fd232668', | ||
| '6b14c622-dad2-44ea-82bc-2dd4010364d5', | ||
| '03c34a8a-a79a-4928-9afc-8647eefabdb1', | ||
| ] |
There was a problem hiding this comment.
I would make this more flexible:
export const SH_ESERVICES_TO_HIDE: Partial<Record<PagoPAEnvVars['STAGE'], Array<string>>> = {
ATT: ['9b6993ee-60e3-4901-9a32-e6987d690ec4'],
UAT: [
'7ab0a0fc-7d22-4007-b2f3-fddd68fe2f17',
'e8c087eb-627b-4488-a9b7-65b70fd1301b',
'407edf51-23b5-462b-af6e-128bbaa4d9ff',
'3b0fbe47-2e2c-4d8b-9cff-b2381c92d003',
'260e45e1-9a61-49d6-8b6d-da0643da68ac',
'a2b84a6e-34cf-44ca-85a4-de21fd232668',
'6b14c622-dad2-44ea-82bc-2dd4010364d5',
'03c34a8a-a79a-4928-9afc-8647eefabdb1',
],
}This would also semplify the logic that uses those
Comment on lines
24
to
35
| {(STAGE === 'ATT' && | ||
| SH_ESERVICES_ATT_TEMP.some( | ||
| (shEservice) => shEservice === eservice.activeDescriptor?.id | ||
| )) || | ||
| (STAGE === 'UAT' && | ||
| SH_ESERVICES_UAT_TEMP.some( | ||
| (shEservice) => shEservice === eservice.activeDescriptor?.id | ||
| )) ? ( | ||
| <CatalogCard key={eservice.activeDescriptor?.id} eservice={eservice} disabled={true} /> | ||
| ) : ( | ||
| <CatalogCard key={eservice.activeDescriptor?.id} eservice={eservice} disabled={false} /> | ||
| )} |
There was a problem hiding this comment.
Suggested change
| {(STAGE === 'ATT' && | |
| SH_ESERVICES_ATT_TEMP.some( | |
| (shEservice) => shEservice === eservice.activeDescriptor?.id | |
| )) || | |
| (STAGE === 'UAT' && | |
| SH_ESERVICES_UAT_TEMP.some( | |
| (shEservice) => shEservice === eservice.activeDescriptor?.id | |
| )) ? ( | |
| <CatalogCard key={eservice.activeDescriptor?.id} eservice={eservice} disabled={true} /> | |
| ) : ( | |
| <CatalogCard key={eservice.activeDescriptor?.id} eservice={eservice} disabled={false} /> | |
| )} | |
| <CatalogCard | |
| key={eservice.activeDescriptor?.id} | |
| eservice={eservice} | |
| disabled={!!SH_ESERVICES_TO_HIDE[STAGE]?.includes(eservice.id)} | |
| /> |
There was a problem hiding this comment.
Are the SH_ESERVICES_TO_HIDE eservice or descriptor ids?
Co-authored-by: Carmine Porricelli <[email protected]>
…om:pagopa/pdnd-interop-frontend into feature/pin-5752-hide-signal-hub-eservices
Carminepo2
requested changes
Dec 10, 2024
Comment on lines
81
to
119
| {disabled ? ( | ||
| <Tooltip open={disabled ? undefined : false} title={t('subscribeTooltip')} arrow> | ||
| <span> | ||
| <Link | ||
| as="button" | ||
| size="small" | ||
| variant="contained" | ||
| to="SUBSCRIBE_CATALOG_VIEW" | ||
| params={{ | ||
| eserviceId: eservice.id, | ||
| descriptorId: eservice.activeDescriptor?.id ?? '', | ||
| }} | ||
| onFocusVisible={handlePrefetch} | ||
| color="primary" | ||
| disabled={disabled} | ||
| > | ||
| {tCommon('actions.inspect')} | ||
| </Link> | ||
| </span> | ||
| </Tooltip> | ||
| ) : ( | ||
| <span> | ||
| <Link | ||
| as="button" | ||
| size="small" | ||
| variant="contained" | ||
| to="SUBSCRIBE_CATALOG_VIEW" | ||
| params={{ | ||
| eserviceId: eservice.id, | ||
| descriptorId: eservice.activeDescriptor?.id ?? '', | ||
| }} | ||
| onFocusVisible={handlePrefetch} | ||
| color="primary" | ||
| disabled={disabled} | ||
| > | ||
| {tCommon('actions.inspect')} | ||
| </Link> | ||
| </span> | ||
| )} |
There was a problem hiding this comment.
There is no need to put the ternary here
martinaCampoli
changed the title
Carminepo2
approved these changes
Dec 10, 2024
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added check on eservices ids to hide signal hub eservices