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: add all speakers button #192

Merged
merged 4 commits into from
Sep 22, 2023

Conversation

Sylvie-Wxr
Copy link
Contributor

@Sylvie-Wxr Sylvie-Wxr commented Sep 12, 2023

Add all speakers button

solves #163

image

image

@netlify
Copy link

netlify bot commented Sep 12, 2023

Deploy Preview for peaceful-ramanujan-288045 ready!

Name Link
🔨 Latest commit e15f6a3
🔍 Latest deploy log https://app.netlify.com/sites/peaceful-ramanujan-288045/deploys/650a1445e09b580008e5c79c
😎 Deploy Preview https://deploy-preview-192--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.

@Sylvie-Wxr Sylvie-Wxr changed the title feat: Add all speakers button feat: add all speakers button Sep 12, 2023
@AceTheCreator
Copy link
Member

Nicely done @Sylvie-Wxr

Just a few changes are needed.

  • On the desktop view, there needs to be spacing between the buttons
  • On the mobile view, the active nav background color needs to be the gradient color

@Sylvie-Wxr
Copy link
Contributor Author

Sylvie-Wxr commented Sep 12, 2023

Nicely done @Sylvie-Wxr

Just a few changes are needed.

  • On the desktop view, there needs to be spacing between the buttons
  • On the mobile view, the active nav background color needs to be the gradient color

Hey @AceTheCreator , i have fixed the spacing.
For "active nav", do you mean the selected dropdown? Currently in the live site, it is not gradient color. Should we > change this in this PR?

@Sylvie-Wxr
Copy link
Contributor Author

Hey @AceTheCreator, should we do the following in a separate issue/PR and merge this first?

On the mobile view, the active nav background color needs to be the gradient color

@AceTheCreator
Copy link
Member

@Sylvie-Wxr you can do it on the same PR

@Sylvie-Wxr
Copy link
Contributor Author

Hi @AceTheCreator , is this the color you meant?

image

@AceTheCreator
Copy link
Member

@Sylvie-Wxr, what I mean is the Button itself should be colored :)

@Sylvie-Wxr
Copy link
Contributor Author

image

@AceTheCreator Like this?

@AceTheCreator
Copy link
Member

Yea @Sylvie-Wxr Also you should also include color on the border :)

@Sylvie-Wxr
Copy link
Contributor Author

Sylvie-Wxr commented Sep 19, 2023

Yea @Sylvie-Wxr Also you should also include color on the border :)

Hi @AceTheCreator I have removed the border. If there are no other changes required, can we merge this PR?

image

@AceTheCreator
Copy link
Member

LGTM!!!

@AceTheCreator AceTheCreator merged commit 39e3f66 into asyncapi:master Sep 22, 2023
12 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.

2 participants