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

added text change width and shape of button #575

Merged
merged 4 commits into from
Aug 13, 2022

Conversation

markarik
Copy link
Contributor

From this link on Google Button Generator
there are attributes from the generated code which would be essential for this package to make it atlist customizable as much as possible

Below is a screenshot of the demo page after working on the attributes

Screenshot (38)

@kattoshi
Copy link
Contributor

I would like to request the following items
theme
logo_alignmnt
locale

@markarik
Copy link
Contributor Author

For those am still testing on them, once comfortable will push ASAP

@markarik
Copy link
Contributor Author

@kattoshi Have managed to add theme and logo_alignment
unfortunately on locale have tried testing it but not working on it,
Have opted to leave it as from the docs it's written if the locale is left as empty, the button should pick the browsers default locale

@Heatmanofurioso
Copy link
Collaborator

@markarik Thanks a lot for your contribution here. Would this be fine to be merged by you? Or would you try to add any more properties to it? The PR looks good by me

@kattoshi
Copy link
Contributor

@markarik Thank you. the test is working fine.
image
image
image

Copy link
Contributor

@kattoshi kattoshi left a comment

Choose a reason for hiding this comment

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

Isn't 'package-lock.json' unnecessary in PR?

Comment on lines +38 to +42
Promise.resolve(this.locale).then((value) => {

console.log("value", value);
})

Copy link
Contributor

Choose a reason for hiding this comment

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

degrades performance

<div class="card-block d-flex justify-content-center">
<asl-google-signin-button class="mx-1"></asl-google-signin-button>

<asl-google-signin-button class="mx-5"></asl-google-signin-button>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think "mx-1" is better

@markarik
Copy link
Contributor Author

@markarik Thanks a lot for your contribution here. Would this be fine to be merged by you? Or would you try to add any more properties to it? The PR looks good by me

Am humbled,, I think am not in a position to merge it

@markarik
Copy link
Contributor Author

@markarik Thank you. the test is working fine. image image image

Great thanks ,, I suppose on my laptop it just dint want to work at the moment

@Heatmanofurioso
Copy link
Collaborator

@markarik Thanks a lot for your contribution here. Would this be fine to be merged by you? Or would you try to add any more properties to it? The PR looks good by me

Am humbled,, I think am not in a position to merge it

I can merge it. What I meant was to ask if you intended to make any more commits to this PR.
Since you've made some changes since it's initial opening

@markarik
Copy link
Contributor Author

@markarik Thanks a lot for your contribution here. Would this be fine to be merged by you? Or would you try to add any more properties to it? The PR looks good by me

Am humbled,, I think am not in a position to merge it

I can merge it. What I meant was to ask if you intended to make any more commits to this PR. Since you've made some changes since it's initial opening

Yes you can merge it, I don't intend to add anything else

@Heatmanofurioso Heatmanofurioso merged commit 3a4a691 into abacritt:master Aug 13, 2022
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