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

fix: subscription button modified #217 #222

Merged
merged 8 commits into from
Oct 23, 2023

Conversation

AyushmanSinghRajawat
Copy link
Contributor

fixes #217
hey, @asyncapi-bot I have modified the subscription button by using the existing Button component so it will have same styling like other buttons in the app and i have also provided relevant top margin.
I had also updated caniuse-lite to it's latest version as otherwise it was not working.
I am new to open source and I enjoyed working on this issue, hope you like my solution.
Please let me know your thoughts and suggestions.

@netlify
Copy link

netlify bot commented Oct 11, 2023

Deploy Preview for peaceful-ramanujan-288045 ready!

Name Link
🔨 Latest commit 7d8c828
🔍 Latest deploy log https://app.netlify.com/sites/peaceful-ramanujan-288045/deploys/653686c8b4b0e900093ef7f7
😎 Deploy Preview https://deploy-preview-222--peaceful-ramanujan-288045.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@AceTheCreator AceTheCreator changed the title subscription button modified #217 fix: subscription button modified #217 Oct 11, 2023
@AceTheCreator
Copy link
Member

@Mayaleeeee what do you think?

@AyushmanSinghRajawat
Copy link
Contributor Author

I think my solution modifies the subscription button to look exactly like other buttons in the project. If you have any feedback please let me know I will work on that.

@Mayaleeeee
Copy link
Member

@Mayaleeeee what do you think?

Thank you @AyushmanSinghRajawat

It looks perfect on the web but is not well positioned on the mobile version.

It should be centre-aligned with the same length as the "All" button under the speakers' section @AyushmanSinghRajawat.
Screenshot 2023-10-12 232708

cc @AceTheCreator, I noticed that the buttons on the mobile version of the website are different in length. In particular, they should all be the same length as the "ALL" button under the speakers' section. Should I open a separate issue, or can we handle it under this PR?

@Mayaleeeee
Copy link
Member

I think my solution modifies the subscription button to look exactly like other buttons in the project. If you have any feedback, please let me know I will work on that.

Hi @AyushmanSinghRajawat, thank you for working on this. I have reviewed your work and provided my feedback.

@AyushmanSinghRajawat
Copy link
Contributor Author

Thank you @Mayaleeeee for reviewing my work, I can center-align the subscribe button with same length as "All" button. Should I also change the length of all buttons in mobile version? if yes, please let me know, i will make the changes ASAP.

@Mayaleeeee
Copy link
Member

Thank you @Mayaleeeee for reviewing my work, I can center-align the subscribe button with same length as "All" button. Should I also change the length of all buttons in mobile version? if yes, please let me know, i will make the changes ASAP.

Yes, please. You can do that.

wdy @AceTheCreator

@AceTheCreator
Copy link
Member

Thank you @Mayaleeeee for reviewing my work, I can center-align the subscribe button with same length as "All" button. Should I also change the length of all buttons in mobile version? if yes, please let me know, i will make the changes ASAP.

This should be in a different PR

@Mayaleeeee
Copy link
Member

Thank you @Mayaleeeee for reviewing my work, I can center-align the subscribe button with same length as "All" button. Should I also change the length of all buttons in mobile version? if yes, please let me know, i will make the changes ASAP.

This should be in a different PR

@AyushmanSinghRajawat

@AyushmanSinghRajawat
Copy link
Contributor Author

ok @AceTheCreator @Mayaleeeee , so for now i should just fix the subscribe button alignment and size, right? and can I raise a new issue for the new PR? I am new to open source so I truly appreciate any guidance from your side.

@Mayaleeeee
Copy link
Member

ok @AceTheCreator @Mayaleeeee , so for now i should just fix the subscribe button alignment and size, right? and can I raise a new issue for the new PR? I am new to open source so I truly appreciate any guidance from your side.

Yeah, adjust the alignment and size of the subscribe button in this PR

And submit another PR for fixing the sizing of all buttons in the mobile version.

Please, feel free to reach out if you require any further assistance.

@AyushmanSinghRajawat
Copy link
Contributor Author

image
I have fixed the alignment and size for the button please review my work and merge my PR.

@AceTheCreator
Copy link
Member

@Mayaleeeee is this okay?

@Mayaleeeee
Copy link
Member

@Mayaleeeee is this okay?

Yes, boss.

Thank you @AyushmanSinghRajawat

@AyushmanSinghRajawat
Copy link
Contributor Author

AyushmanSinghRajawat commented Oct 14, 2023

Kindly merge my PR @AceTheCreator @Mayaleeeee

@AceTheCreator
Copy link
Member

@AyushmanSinghRajawat so sorry for the late response 🙏🏾

Can you make this branch up-to-date with the master? so I can go ahead and merge it ✌🏾

@AyushmanSinghRajawat
Copy link
Contributor Author

Hey @AceTheCreator I have updated this branch. You can merge this PR now.

@AyushmanSinghRajawat
Copy link
Contributor Author

hey, @AceTheCreator kindly merge my PR.

@AceTheCreator
Copy link
Member

hey, @AceTheCreator kindly merge my PR.

@AyushmanSinghRajawat update your branch and remove the changes you made to package.lock

@AyushmanSinghRajawat
Copy link
Contributor Author

@AceTheCreator I have updated this branch and also removed changes from package-lock. You can merge my PR now.

@AceTheCreator AceTheCreator merged commit 5c89e24 into asyncapi:master Oct 23, 2023
11 checks passed
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.

Inconsistent styling of the subscribe button on the conference website
3 participants