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.

Sign up for GitHub

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

Jump to bottom

fix: added links to all Expenses Breakdown cards in Finance. #2563

Merged
merged 8 commits into from
Mar 7, 2024
Merged

fix: added links to all Expenses Breakdown cards in Finance. #2563

merged 8 commits into from
Mar 7, 2024

Conversation

sagarkori143
Copy link
Contributor

@sagarkori143 sagarkori143 commented Jan 16, 2024 •
edited
Loading

closes: #2482

  • Added the links to all cards in the financial breakdown section of the Finance page.

  • Added the scaling animation while hovering over the cards.

@netlify Netlify
Copy link

netlify bot commented Jan 16, 2024 •
edited
Loading

✅ Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 5e4536d
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/65e93af33e3fab00082a88b5
😎 Deploy Preview https://deploy-preview-2563--asyncapi-website.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.

@asyncapi-bot
Copy link
Contributor

asyncapi-bot commented Jan 16, 2024 •
edited
Loading

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 36
🟢 Accessibility 98
🟢 Best practices 92
🟢 SEO 100
🔴 PWA 33

Lighthouse ran on https://deploy-preview-2563--asyncapi-website.netlify.app/

@sagarkori143 sagarkori143 changed the title added links fix: added links to all Expenses Breakdown cards in Finance. Jan 16, 2024
@sagarkori143 sagarkori143 mentioned this pull request Jan 16, 2024
Closed
@sagarkori143
Copy link
Contributor Author

sagarkori143 commented Jan 16, 2024 •
edited
Loading

Hi @akshatnema @derberg . I have implemented the required changes from scratch again. Please have a look. Waiting for a merge notification eagerly. Thanks!

@sagarkori143 sagarkori143 mentioned this pull request Jan 26, 2024
Merged
@sagarkori143
Copy link
Contributor Author

@akshatnema @derberg A friendly reminder. Please have a look and let me know if any changes required. Thanks!

@anshgoyalevil
Copy link
Member

anshgoyalevil commented Jan 29, 2024 •
edited
Loading

@Mayaleeeee Can you please verify the new links if they are correct for each Card

You can view the new page here: https://deploy-preview-2563--asyncapi-website.netlify.app/finance

@Mayaleeeee
Copy link
Member

Mayaleeeee commented Jan 30, 2024 •
edited
Loading

@Mayaleeeee Can you please verify the new links if they are correct for each Card

You can view the new page here: https://deploy-preview-2563--asyncapi-website.netlify.app/finance

Thank you, @anshgoyalevil @sagarkori143

The links for the Mentorship, Bounty Program, and Swag Store cards look excellent, but I have a suggestion regarding the Events, Hiring and Services links.

  • Events - Instead of directing users to a generic AsyncAPI Events page, it would be better to link to the GitHub discussion on our first AsyncAPI Conf on Tour'22, as the Card explains the AsyncAPI Conferences and not the community events/meetings.

  • Hiring - When I clicked on the Hiring link, it redirected me to my personal dashboard page on Open Collective. It would be more appropriate to link to the Open Collective page that shows Thulie's payment or, even better, to the GitHub discussion on hiring Thulie.

  • Services - This link led me to the Zoom pricing page, which needs to be corrected. Instead, it should direct users to the GitHub discussion on this topic.

What do you think, @akshatnema and @derberg?

Lastly, since the cards will be clickable, I suggest adding an interaction feature, such as an elevation effect, once users hover over the cards.

@sagarkori143
Copy link
Contributor Author

sagarkori143 commented Jan 31, 2024 •
edited
Loading

@Mayaleeeee Give me a thumbs up if This link is correct for Services card.

@sagarkori143
Copy link
Contributor Author

And This for Events.

@sagarkori143
Copy link
Contributor Author

sagarkori143 commented Feb 7, 2024 •
edited
Loading

@Mayaleeeee @anshgoyalevil Friendly reminder!

@anshgoyalevil
Copy link
Member

@sagarkori143 Have you made the changes suggested by @Mayaleeeee

@sagarkori143
Copy link
Contributor Author

sagarkori143 commented Feb 12, 2024 •
edited
Loading

Hey @anshgoyalevil Yes! I have made the changes requested by @Mayaleeeee . Please take a look.

@sagarkori143
Copy link
Contributor Author

Friendly reminder.
@Mayaleeeee @anshgoyalevil

@sagarkori143
Copy link
Contributor Author

sagarkori143 commented Feb 17, 2024 •
edited
Loading

Hey @anshgoyalevil . Please check this out. This PR is a ready-to-go and if it is merged, the incomplete 4 or 5 PRs related to this issue will also be closed and it will be a PR cleanup for the repo. So kindly give a little time and review it.

@sagarkori143
Copy link
Contributor Author

Friendly reminder.
@anshgoyalevil @Mayaleeeee @sambhavgupta0705

@Mayaleeeee
Copy link
Member

Mayaleeeee commented Feb 27, 2024 •
edited
Loading

And This for Events.

Is this the updated link for the 'Events'? The link on the deploy-preview took me to a different page.

@sagarkori143
Copy link
Contributor Author

And This for Events.

Is this the updated link for the 'Events'? The link on the deploy-preview took me to a different page.

which one of these two do you think will be correct? I will make sure that the link selected by you will be reflected in the deployment soon.

@akshatnema
Copy link
Member

Is this the updated link for the 'Events'? The link on the deploy-preview took me to a different page.

@Mayaleeeee Kindly provide the link for Events card or verify the current one in the PR, if it is correct.

@Mayaleeeee
Copy link
Member

And This for Events.

Is this the updated link for the 'Events'? The link on the deploy-preview took me to a different page.

which one of these two do you think will be correct? I will make sure that the link selected by you will be reflected in the deployment soon.

This one
https://github.com/orgs/asyncapi/discussions/381

@akshatnema
Copy link
Member

akshatnema commented Feb 28, 2024 •
edited
Loading

@Mayaleeeee Kindly review the PR. I've updated the links.

@sagarkori143 Kindly update the PR description.

@sagarkori143
Copy link
Contributor Author

Hey @akshatnema @Mayaleeeee .
I think it's done from my side. PTAL .

sambhavgupta0705
sambhavgupta0705 approved these changes Mar 5, 2024
View reviewed changes
Copy link
Member

@sambhavgupta0705 sambhavgupta0705 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

anshgoyalevil
anshgoyalevil approved these changes Mar 5, 2024
View reviewed changes
Copy link
Member

@anshgoyalevil anshgoyalevil left a comment

Choose a reason for hiding this comment

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

awesome @sagarkori143 🌟

components/FinancialSummary/ExpenseBreakdown.js Outdated Show resolved Hide resolved
@anshgoyalevil
Copy link
Member

/rtm

@sambhavgupta0705
Copy link
Member

@anshgoyalevil it needs @akshatnema's approval as he has requested changes

@sagarkori143
Copy link
Contributor Author

Hey @sambhavgupta0705 @anshgoyalevil Why is the " Run tests / cypress-run (4) (pull_request) " failing in all pull requests.

@anshgoyalevil
Copy link
Member

Hey @sambhavgupta0705 @anshgoyalevil Why is the " Run tests / cypress-run (4) (pull_request) " failing in all pull requests.

ref: #2737

@akshatnema
Copy link
Member

@sambhavgupta0705 kindly get a review from @Mayaleeeee as well.

Mayaleeeee
Mayaleeeee reviewed Mar 7, 2024
View reviewed changes
Copy link
Member

@Mayaleeeee Mayaleeeee left a comment

Choose a reason for hiding this comment

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

Weldone @sagarkori143
I'm approving this.

components/FinancialSummary/ExpenseBreakdown.js Outdated Show resolved Hide resolved
@sambhavgupta0705
Copy link
Member

/rtm

@Mayaleeeee Mayaleeeee self-requested a review March 7, 2024 09:37
@asyncapi-bot asyncapi-bot merged commit 3c8c08d into asyncapi:master Mar 7, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mayaleeeee Mayaleeeee Mayaleeeee approved these changes

@anshgoyalevil anshgoyalevil anshgoyalevil approved these changes

@sambhavgupta0705 sambhavgupta0705 sambhavgupta0705 approved these changes

@akshatnema akshatnema akshatnema approved these changes

@derberg derberg Awaiting requested review from derberg derberg is a code owner

@magicmatatjahu magicmatatjahu Awaiting requested review from magicmatatjahu magicmatatjahu is a code owner

@asyncapi-bot-eve asyncapi-bot-eve Awaiting requested review from asyncapi-bot-eve asyncapi-bot-eve is a code owner

Assignees
No one assigned
Labels
ready-to-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add links to components of expense breakdown
6 participants
@sagarkori143 @asyncapi-bot @anshgoyalevil @Mayaleeeee @akshatnema @sambhavgupta0705