chore: [PE-678] CGN partner detail change go to website from button to list item #6300

freddi301 · 2024-10-17T15:43:28Z

Short description

CGN partner detail change go to website from button to list item.

List of changes proposed in this pull request

remove bottom button
update translation files
add list item with same behavior as button had

How to test

login into the app
go to cgn card brought wallet (add it if necessary)
go to opportunities list
trough categories list or partner list go to a partner detail screen
check if the new design at the bottom of the screen

pagopa-github-bot · 2024-10-17T15:44:54Z

Affected stories

⚙️ PE-678: [FE] Implementazione link "Vai al sito del partner" nella schermata dettaglio partner
subtask of
- 🌟 PE-676: Sistemare cta "Vai al sito del partner" nella schermata di dettaglio partner

Generated by 🚫 dangerJS against b11099b

codecov · 2024-10-17T16:00:04Z

Codecov Report

Attention: Patch coverage is 0% with 20 lines in your changes missing coverage. Please review.

Project coverage is 47.01%. Comparing base (4f204b4) to head (b11099b).
Report is 627 commits behind head on master.

Files with missing lines	Patch %	Lines
.../cgn/screens/merchants/CgnMerchantDetailScreen.tsx	0.00%	20 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6300      +/-   ##
==========================================
- Coverage   48.42%   47.01%   -1.42%     
==========================================
  Files        1488     1803     +315     
  Lines       31617    36547    +4930     
  Branches     7669     8652     +983     
==========================================
+ Hits        15311    17181    +1870     
- Misses      16238    19309    +3071     
+ Partials       68       57      -11

Files with missing lines	Coverage Δ
.../cgn/screens/merchants/CgnMerchantDetailScreen.tsx	`7.27% <0.00%> (-5.55%)`	⬇️

... and 1420 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0df6de...b11099b. Read the comment docs.

LeleDallas

A little summary of request changes:

Update sentences
Simplify the conditional rendering for better readability
Replace array index with unique identifiers
Remove redundant fragment that contain only one (line 132 to 197)

LeleDallas · 2024-10-17T16:22:54Z

locales/it/index.yml

@@ -2717,7 +2717,7 @@ bonus:
        description: Descrizione
        contactInfo: Contatti e informazioni
      cta:
-        website: Vai al sito del partner
+        website: Visita al sito del partner


Update the string to: Visita il sito del partner

OMG such a typo, thanks!

LeleDallas · 2024-10-17T16:25:47Z

locales/en/index.yml

@@ -2717,7 +2717,7 @@ bonus:
        description: Description
        contactInfo: Addresses
      cta:
-        website: Go to partner's website
+        website: Visit to partner's website


Update the string to: Visit the partner's website

LeleDallas · 2024-10-17T16:27:51Z

ts/features/bonus/cgn/screens/merchants/CgnMerchantDetailScreen.tsx

@@ -188,7 +139,7 @@ const CgnMerchantDetailScreen = () => {
            snapToEnd={false}
            contentContainerStyle={{
              flexGrow: 1,
-              paddingBottom: safeBottomAreaHeight
+              paddingBottom: bottomMargin


Rename it to paddingBottom and pass it as shorthand

LeleDallas · 2024-10-17T16:31:43Z

ts/features/bonus/cgn/screens/merchants/CgnMerchantDetailScreen.tsx

+              {showGotToWebsite && showAddresses ? <Divider /> : null}
+              {merchantDetail.value.addresses?.map((address, index) => (
+                <CgnAddressListItem
+                  item={address}


Using array indices as keys in React can lead to issues with component state and performance, especially when the list can change. Instead, you should use a unique identifier for each item.

unfortunately address is as embedded value, no unique identifiers available from backend as of now

I saw we had installed the uuid library. In that case I suggest you to use it to generate unique keys for each item instead of relying on array indices

that would be using random keys, index is just as good as a random value

Oh correct. Could the address serve as a unique identifier for each merchant, or do you think that could be problematic due to multiple businesses sharing the same location?

I don't have a strong opinion on this, but since there can only be up to 3 addresses, performance edge cases are not a concern here. Especially since the backend doesn't provide an ID, I think using the index or a UUID would be equally fine in this case. However, I would avoid using the address name as a key, because—even though it's unlikely—there's still a very small chance that a merchant could enter duplicate addresses (even by mistake).

What do you think?

Both using the index or a UUID would work, but I would lean toward UUID for flexibility and to avoid potential issues with reordering or modifying the address list

At the moment, there is no action involving modifying, reordering, or expanding the data (there will always be a maximum of 3 addresses), so using the index is not incorrect. However, to be future-proof, in case at some distant point we decide to introduce a more dynamic way of handling these addresses, like reordering or similar changes, we could consider using UUIDs. What do you think?

We are on the same page

Should be the same. Good catch! I think we can move on now!

LeleDallas · 2024-10-17T16:39:35Z

ts/features/bonus/cgn/screens/merchants/CgnMerchantDetailScreen.tsx

-                merchantDetail.value.addresses,
-                merchantDetail.value.allNationalAddresses
-              )}
+              {showAddresses || showGotToWebsite ? (


I suggest to replace the ternary operator (? :) with the logical AND (&&) operator in our conditional renderings for better readability and simplicity.

ch: button to list item

a3c5ff3

freddi301 added the Carta Giovani Nazionale label Oct 17, 2024

freddi301 requested review from thisisjp and a team as code owners October 17, 2024 15:43

freddi301 temporarily deployed to dev October 17, 2024 15:43 — with GitHub Actions Inactive

LeleDallas reviewed Oct 17, 2024

View reviewed changes

code review

b038742

freddi301 temporarily deployed to dev October 18, 2024 09:35 — with GitHub Actions Inactive

LeleDallas approved these changes Oct 18, 2024

View reviewed changes

Merge branch 'master' into PE-678-cgn-partner-go-to-website

b11099b

freddi301 temporarily deployed to dev October 18, 2024 15:21 — with GitHub Actions Inactive

Hantex9 merged commit 8fce513 into master Oct 18, 2024
13 checks passed

Hantex9 deleted the PE-678-cgn-partner-go-to-website branch October 18, 2024 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: [PE-678] CGN partner detail change go to website from button to list item #6300

chore: [PE-678] CGN partner detail change go to website from button to list item #6300

freddi301 commented Oct 17, 2024

pagopa-github-bot commented Oct 17, 2024 •

edited by jira bot

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading

LeleDallas left a comment

LeleDallas Oct 17, 2024

freddi301 Oct 18, 2024 •

edited

Loading

LeleDallas Oct 17, 2024

LeleDallas Oct 17, 2024

LeleDallas Oct 17, 2024

freddi301 Oct 18, 2024

LeleDallas Oct 18, 2024

freddi301 Oct 18, 2024

LeleDallas Oct 18, 2024

Hantex9 Oct 18, 2024

LeleDallas Oct 18, 2024

Hantex9 Oct 18, 2024

LeleDallas Oct 18, 2024

LeleDallas Oct 18, 2024

LeleDallas Oct 17, 2024

chore: [PE-678] CGN partner detail change go to website from button to list item #6300

chore: [PE-678] CGN partner detail change go to website from button to list item #6300

Conversation

freddi301 commented Oct 17, 2024

Short description

List of changes proposed in this pull request

How to test

pagopa-github-bot commented Oct 17, 2024 • edited by jira bot Loading

Affected stories

codecov bot commented Oct 17, 2024 • edited Loading

Codecov Report

LeleDallas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freddi301 Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pagopa-github-bot commented Oct 17, 2024 •

edited by jira bot

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading

freddi301 Oct 18, 2024 •

edited

Loading