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

Make container ID as optional prop #110

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

luisguerra
Copy link

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

@luisguerra
Copy link
Author

@AleFossati
Copy link
Contributor

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

@AdaltonLeite
Copy link
Contributor

@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 😄

onReady = onReadyDefault,
customization,
locale,
divId = 'brandBrick_container',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
divId = 'brandBrick_container',
id = 'brandBrick_container',

@@ -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>;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

valueProp: 'payment_methods_logos',
},
}}
divId="customBrandBrick_container"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
divId="customBrandBrick_container"
id="customBrandBrick_container"

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

Choose a reason for hiding this comment

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

Suggested change
divId,
divId: id,

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

Choose a reason for hiding this comment

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

Suggested change
divId?: string;
id?: string;

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.

3 participants