Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Bonus Pagamenti Digitali): [#175797662] Display bancomat abi logo #2442

Merged
merged 15 commits into from
Nov 27, 2020

Conversation

Undermaken
Copy link
Contributor

@Undermaken Undermaken commented Nov 26, 2020

Short description

This PR displays the bancomat abi logo as a remote resource
A list of abi images are been added to our CDN

how to test:

CDN will be sync tomorrow (27th November) so you can change your .env settings as follows
CONTENT_REPO_URL='https://raw.githubusercontent.com/pagopa/io-services-metadata/master'

note

for bancomat displayed as a toggle item, we prefer the bancomat logo instead of abi logo
see https://www.pivotaltracker.com/story/show/175797662/comments/220045266

wallet preview added bancomat detail
Simulator Screen Shot - iPhone 11 - 2020-11-26 at 19 22 08 Simulator Screen Shot - iPhone 11 - 2020-11-26 at 19 23 41
adding bancomat detail cashback detail
Simulator Screen Shot - iPhone 11 - 2020-11-26 at 19 32 04 Simulator Screen Shot - iPhone 11 - 2020-11-26 at 20 01 38

@pagopa-github-bot pagopa-github-bot changed the title [#175797662] Display abi images feat(Bonus Pagamenti Digitali): [#175797662] Display abi images Nov 26, 2020
@pagopa-github-bot
Copy link
Collaborator

pagopa-github-bot commented Nov 26, 2020

Affected stories

  • 🌟 #175797662: Come CIT voglio poter vedere il logo della mia banca legato alla mia carta PagoBancomat

Generated by 🚫 dangerJS against 6a1ebb3

@Undermaken Undermaken changed the title feat(Bonus Pagamenti Digitali): [#175797662] Display abi images feat(Bonus Pagamenti Digitali): [#175797662] Display bancomat abi logo Nov 26, 2020
@codecov-io
Copy link

codecov-io commented Nov 26, 2020

Codecov Report

Merging #2442 (98e112a) into master (cb168f1) will increase coverage by 0.02%.
The diff coverage is 61.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2442      +/-   ##
==========================================
+ Coverage   50.07%   50.10%   +0.02%     
==========================================
  Files         672      673       +1     
  Lines       19170    19181      +11     
  Branches     3752     3751       -1     
==========================================
+ Hits         9600     9611      +11     
  Misses       9524     9524              
  Partials       46       46              
Impacted Files Coverage Δ
...ActivationToggle/BpdPaymentMethodToggleFactory.tsx 54.54% <0.00%> (-5.46%) ⬇️
ts/types/pagopa.ts 100.00% <ø> (ø)
...t/bancomat/component/bancomatCard/BancomatCard.tsx 50.00% <33.33%> (ø)
...bancomat/screens/add-pans/AddBancomatComponent.tsx 76.00% <33.33%> (+4.00%) ⬆️
...allet/bancomat/component/BancomatWalletPreview.tsx 60.00% <37.50%> (+3.33%) ⬆️
ts/utils/image.ts 100.00% <100.00%> (ø)
ts/utils/paymentMethod.ts 81.25% <100.00%> (+1.58%) ⬆️

Continue to review full report at Codecov.

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

@Undermaken Undermaken marked this pull request as ready for review November 26, 2020 19:46
@Undermaken Undermaken marked this pull request as draft November 27, 2020 07:36
@Undermaken Undermaken marked this pull request as ready for review November 27, 2020 08:12
Comment on lines 123 to 125
caption: getTitleFromBancomat(bancomat, abiList),
icon: getImageFromPaymentMethod(bancomat)
icon: getImageFromPaymentMethod(bancomat),
listIcon: pagoBancomatImage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The icon field should represent the icon that in the case of the bancomat should remain the "bancomat" icon and we can avoid to add another field listIcon.

Could we move the new bank logo information in BancomatPaymentMethod.abiInfo.logoUrl?
All the existing code should be fine after this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a smarter injection 👍🏼
I'm going to try....

Copy link
Contributor Author

@Undermaken Undermaken Nov 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabriziofff
I changed the code as required: now the abi selector gives back an enhanced abi with logoUrl pointing out CDN
In this way if we need the abi logo, that info is available inside abi and not only in the enhanced bancomat (enhanced bancomat includes the abi itself)

@Undermaken Undermaken marked this pull request as draft November 27, 2020 12:16
@Undermaken Undermaken marked this pull request as ready for review November 27, 2020 12:30
@fabriziofff fabriziofff merged commit 3f88d42 into master Nov 27, 2020
@fabriziofff fabriziofff deleted the 175797662-display-abi-image branch November 27, 2020 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants