Make container ID as optional prop #110

luisguerra · 2024-10-21T07:42:07Z

This PR enables the ability to modify the div to avoid collision with existing ID's or define multiple elements

luisguerra · 2024-10-23T02:26:26Z

cc @francineemilia @icaldana @AdaltonLeite @AleFossati @lemazzo

AleFossati · 2024-10-23T12:15:43Z

Olá @luisguerra, assim que possível faremos a revisão e teste das alterações.
Obrigado pela contribuição!

AdaltonLeite · 2024-10-23T12:36:12Z

@luisguerra obrigado por contribuir com o projeto, excelente sugestão.

Tenho só um ponto que penso que podemos alterar. O nome da propriedade divId, penso que podemos alterar para somente id.

Como é um elemento customizado do React não teria conflito com outras propriedades. Além de ser padrão a utilização somente da prop id nos elementos HTML. Um outro ponto também é que se em um futuro a gente troca o elemento de uma div para qualquer outro(ej: section), geraríamos uma quebra de contrato para trocar essa propriedade, já que ela não seria mais consistente com a proposta.

O que você acha? Podemos mudar esse nome da prop para ser mais consistente no futuro? Alterando isso a gente já aprova o PR 😄

AdaltonLeite · 2024-10-23T12:36:31Z

src/bricks/brand/index.tsx

+  onReady = onReadyDefault,
+  customization,
+  locale,
+  divId = 'brandBrick_container',


Suggested change

divId = 'brandBrick_container',

id = 'brandBrick_container',

AdaltonLeite · 2024-10-23T12:36:51Z

src/bricks/brand/index.tsx

@@ -50,7 +55,7 @@ const Brand = ({ onReady = onReadyDefault, customization, locale }: TBrand) => {
      window.brandBrickController?.unmount();
    };
  }, [customization, onReady]);
-  return <div id="brandBrick_container"></div>;
+  return <div id={divId}></div>;


Suggested change

return <div id={divId}></div>;

return <div id={id}></div>;

AdaltonLeite · 2024-10-23T12:37:01Z

src/bricks/brand/brand.test.tsx

+            valueProp: 'payment_methods_logos',
+          },
+        }}
+        divId="customBrandBrick_container"


Suggested change

divId="customBrandBrick_container"

id="customBrandBrick_container"

AdaltonLeite · 2024-10-23T12:40:57Z

src/bricks/brand/index.tsx

@@ -37,7 +42,7 @@ const Brand = ({ onReady = onReadyDefault, customization, locale }: TBrand) => {
        },
      },
      name: 'brand',
-      divId: 'brandBrick_container',
+      divId,


Suggested change

divId,

divId: id,

AdaltonLeite · 2024-10-23T12:41:05Z

src/bricks/brand/types.ts

+  /**
+   * Optional. Container ID where the Brick will be rendered. Default: 'walletBrick_container'
+   */
+  divId?: string;


Suggested change

divId?: string;

id?: string;

luisguerra · 2024-11-04T02:53:15Z

(Sorry I don't speak Portuguese 😅 , hello From Mexico, so I will write in English)
Thanks for feedback @AdaltonLeite, makes sense. I have made the requested changes

AleFossati

Hello @luisguerra,

I have tested the changes and it looks great, there's only a few changes needed, it seems that the default values in the comments are all referencing wallet brick.

After this fix, I think that we are ready to merge!

AleFossati · 2024-11-04T18:51:12Z

src/bricks/brand/types.ts

@@ -18,6 +18,10 @@ export type TBrand = {
   * @see {@link https://www.mercadopago.com/developers/en/docs/checkout-bricks/additional-content/select-language Bricks language customization} documentation.
   */
  locale?: 'es-AR' | 'es-CL' | 'es-CO' | 'es-MX' | 'es-VE' | 'es-UY' | 'es-PE' | 'pt-BR' | 'en-US';
+  /**
+   * Optional. Container ID where the Brick will be rendered. Default: 'walletBrick_container'


Suggested change

* Optional. Container ID where the Brick will be rendered. Default: 'walletBrick_container'

* Optional. Container ID where the Brick will be rendered. Default: 'brandBrick_container'

AleFossati · 2024-11-04T19:02:20Z

src/bricks/cardPayment/type.ts

@@ -100,6 +100,10 @@ export type TCardPayment = {
     */
    visual?: object;
  };
+  /**
+   * Optional. Container ID where the Brick will be rendered. Default: 'walletBrick_container'


Suggested change

* Optional. Container ID where the Brick will be rendered. Default: 'walletBrick_container'

* Optional. Container ID where the Brick will be rendered. Default: 'cardPaymentBrick_container'

AleFossati · 2024-11-04T19:06:26Z

src/bricks/payment/type.ts

@@ -97,6 +97,10 @@ export type TPaymentType = {
   * @see {@link https://www.mercadopago.com/developers/en/docs/checkout-bricks/additional-content/select-language General Customizations # Select Language} documentation.
   */
  locale?: string;
+  /**
+   * Optional. Container ID where the Brick will be rendered. Default: 'walletBrick_container'


Suggested change

* Optional. Container ID where the Brick will be rendered. Default: 'walletBrick_container'

* Optional. Container ID where the Brick will be rendered. Default: 'paymentBrick_container'

AleFossati · 2024-11-04T19:07:01Z

src/bricks/statusScreen/types.ts

@@ -28,6 +28,10 @@ export interface IStatusScreenBrickSettings extends IStatusScreenBrickCallbacks
   * @see {@link https://www.mercadopago.com/developers/en/docs/checkout-bricks/additional-content/select-language General Customization # Select Language} documentation.
   */
  locale?: 'es-AR' | 'es-CL' | 'es-CO' | 'es-MX' | 'es-VE' | 'es-UY' | 'es-PE' | 'pt-BR' | 'en-US';
+  /**
+   * Optional. Container ID where the Brick will be rendered. Default: 'walletBrick_container'


Suggested change

* Optional. Container ID where the Brick will be rendered. Default: 'walletBrick_container'

* Optional. Container ID where the Brick will be rendered. Default: 'statusScreenBrick_container'

icaldana · 2024-11-13T11:52:17Z

@luisguerra hey, could you please check @AleFossati comments? Thank you a lot!

Make container ID as optional prop

326c216

AdaltonLeite reviewed Oct 23, 2024

View reviewed changes

Change divId for just id

1fb5312

luisguerra requested a review from AdaltonLeite November 4, 2024 02:50

AleFossati requested changes Nov 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make container ID as optional prop #110

Make container ID as optional prop #110

luisguerra commented Oct 21, 2024

luisguerra commented Oct 23, 2024

AleFossati commented Oct 23, 2024

AdaltonLeite commented Oct 23, 2024

AdaltonLeite Oct 23, 2024

AdaltonLeite Oct 23, 2024

AdaltonLeite Oct 23, 2024

AdaltonLeite Oct 23, 2024

AdaltonLeite Oct 23, 2024

luisguerra commented Nov 4, 2024

AleFossati left a comment

AleFossati Nov 4, 2024

AleFossati Nov 4, 2024

AleFossati Nov 4, 2024

AleFossati Nov 4, 2024

icaldana commented Nov 13, 2024

	divId="customBrandBrick_container"
	id="customBrandBrick_container"

	* Optional. Container ID where the Brick will be rendered. Default: 'walletBrick_container'
	* Optional. Container ID where the Brick will be rendered. Default: 'brandBrick_container'

Make container ID as optional prop #110

Are you sure you want to change the base?

Make container ID as optional prop #110

Conversation

luisguerra commented Oct 21, 2024

luisguerra commented Oct 23, 2024

AleFossati commented Oct 23, 2024

AdaltonLeite commented Oct 23, 2024

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

luisguerra commented Nov 4, 2024

AleFossati left a 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

icaldana commented Nov 13, 2024