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: added all required sub pages UI for the community section #1071

Closed
wants to merge 37 commits into from
Closed

feat: added all required sub pages UI for the community section #1071

wants to merge 37 commits into from

Conversation

AceTheCreator
Copy link
Member

@AceTheCreator AceTheCreator commented Nov 2, 2022

This PR shows all community sub-page's user interfaces which includes

  • /community/ambassador-programs
  • /community/contributors
  • /community/events

@netlify
Copy link

netlify bot commented Nov 2, 2022

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 580ddf2
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/6373a87bde162f00090481cf
😎 Deploy Preview https://deploy-preview-1071--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 settings.

@AceTheCreator
Copy link
Member Author

@derberg why's Netlify failing?

@derberg
Copy link
Member

derberg commented Nov 2, 2022

@AceTheCreator I only know what is in the logs:

1:25:09 PM: Failed to compile.
1:25:09 PM: 
1:25:09 PM: ./node_modules/@asyncapi/modelina/lib/esm/helpers/FileHelpers.js
1:25:09 PM: Module not found: Can't resolve 'fs' in '/opt/build/repo/node_modules/@asyncapi/modelina/lib/esm/helpers'
1:25:09 PM: Import trace for requested module:
1:25:09 PM: ./node_modules/@asyncapi/modelina/lib/esm/helpers/index.js
1:25:09 PM: ./node_modules/@asyncapi/modelina/lib/esm/index.js
1:25:09 PM: ./components/modelina/TypeScriptGenerator.js
1:25:09 PM: ./pages/tools/modelina.js
1:25:09 PM: > Build failed because of webpack errors

but why all suddenly there is a problem with modelina, I have no idea 😄 you need to investigate 😄

@asyncapi-bot
Copy link
Contributor

Hello, @derberg! 👋🏼

I'm Genie from the magic lamp. Looks like somebody needs a hand! 🆘

At the moment the following comments are supported in pull requests:

  • /ready-to-merge or /rtm - This comment will trigger automerge of PR in case all required checks are green, approvals in place and do-not-merge label is not added
  • /do-not-merge or /dnm - This comment will block automerging even if all conditions are met and ready-to-merge label is added
  • /autoupdate or /au - This comment will add autoupdate label to the PR and keeps your PR up-to-date to the target branch's future changes. Unless there is a merge conflict or it is a draft PR.

@quetzalliwrites
Copy link
Member

quetzalliwrites commented Nov 12, 2022

HUGE improvements @AceTheCreator, nice job!

I see 4 main bugs to address right now:
(1) You've chosen some light grey gradient for some of the headers that are NOT visible. Let's change these headers to have a readable hue, even if it's black and matches the other headers. (maybe go for a blue/purple gradient hue?)

Screen Shot 2022-11-11 at 6 06 41 PM

Screen Shot 2022-11-11 at 6 09 24 PM

(2) In mobile, the Community goals section has a bug; the events from the PREVIOUS Meeting & Events section are broken and cover all the text and pictures in Community goals area.
Screen Shot 2022-11-11 at 6 09 06 PM

(3) In mobile, there is a HUGE blank space between the Slack and newsletter sections. Not sure what's causing that but it's def not a good thing.
Screen Shot 2022-11-11 at 6 10 08 PM

(4) In the Community menu item for the new /Overview page, we need to fix the punctuation. (I would have fixed it myself but I couldn't find the menu code in the PR. 😂

Let's change it to: Connect, share, and learn.

Screen Shot 2022-11-11 at 6 16 32 PM

@quetzalliwrites
Copy link
Member

Added a dnm label just in case 😄✌🏽

@derberg
Copy link
Member

derberg commented Nov 15, 2022

I thought the idea of having a community branch is that as a result, we will get smaller focused-on-one-thing PRs 😅

@AceTheCreator can you indicate what is the scope of this PR? @akshatnema already looks into code, so I'd like to see from UI perspective, but don't want to provide feedback on things that are planned for another PR.

You could update PR title and description 🙏🏼

@AceTheCreator
Copy link
Member Author

I thought the idea of having a community branch is that as a result, we will get smaller focused-on-one-thing PRs 😅

@AceTheCreator can you indicate what is the scope of this PR? @akshatnema already looks into code, so I'd like to see from UI perspective, but don't want to provide feedback on things that are planned for another PR.

You could update PR title and description 🙏🏼

Yea, I think the reason I decided to go this route of adding other pages is cuz some reviewers think the missing pages weren't cool so decided to work on all the pages so that they can review the UI then I can focus on core functionality in the next PR

@AceTheCreator AceTheCreator changed the title feat: added new pages feat: Added all requited sub pages for the community section Nov 15, 2022
@AceTheCreator AceTheCreator changed the title feat: Added all requited sub pages for the community section feat: Added all requited sub pages UI for the community section Nov 15, 2022
@AceTheCreator AceTheCreator changed the title feat: Added all requited sub pages UI for the community section feat: added all requited sub pages UI for the community section Nov 15, 2022
@AceTheCreator AceTheCreator changed the title feat: added all requited sub pages UI for the community section feat: added all required sub pages UI for the community section Nov 15, 2022
@derberg
Copy link
Member

derberg commented Nov 15, 2022

Interesting. I'm not sure how others, but when I see 👇🏼
Screenshot 2022-11-15 at 17 23 18

then PR goes to the bottom of the list of PRs for review. I saw your PR few days ago and that is what I did with my priority list 😆

you need to find a good balance 😄

@derberg
Copy link
Member

derberg commented Nov 15, 2022

This PR shows all community sub-page's user interfaces which includes
/community/ambassador-programs
/community/contributors
/community/events

/community/ambassador-program

ok, so what is the scope of review here:

  • text?
  • these different colorful components? if they should be there or?
  • how info about ambassadors is displayed?
  • or the fact that you extended/modified original AMBASSADORS_MEMBERS.json

/community/events

it is a dead page

/community/contributors

here I do not even know where to start 😄 as it says contributors but on a list I see ambassadors. These are not the same, and the source of data is totally different.


I have no idea what is the scope of the review 😄

@AceTheCreator
Copy link
Member Author

This PR shows all community sub-page's user interfaces which includes
/community/ambassador-programs
/community/contributors
/community/events

/community/ambassador-program

ok, so what is the scope of review here:

  • text?
  • these different colorful components? if they should be there or?
  • how info about ambassadors is displayed?
  • or the fact that you extended/modified original AMBASSADORS_MEMBERS.json

/community/events

it is a dead page

/community/contributors

here I do not even know where to start 😄 as it says contributors but on a list I see ambassadors. These are not the same, and the source of data is totally different.

I have no idea what is the scope of the review 😄

The scope of review is design and text @derberg. The source of data is temporary for now... I had to use it just to have some output

@AceTheCreator
Copy link
Member Author

This PR shows all community sub-page's user interfaces which includes
/community/ambassador-programs
/community/contributors
/community/events

/community/ambassador-program

ok, so what is the scope of review here:

  • text?
  • these different colorful components? if they should be there or?
  • how info about ambassadors is displayed?
  • or the fact that you extended/modified original AMBASSADORS_MEMBERS.json

/community/events

it is a dead page

You sure? https://deploy-preview-1071--asyncapi-website.netlify.app/community/events

/community/contributors

here I do not even know where to start 😄 as it says contributors but on a list I see ambassadors. These are not the same, and the source of data is totally different.

I have no idea what is the scope of the review 😄

@derberg
Copy link
Member

derberg commented Nov 15, 2022

I just noticed we did not have branch protection and code owners requirement for review for `community branch. Solved

@quetzalliwrites
Copy link
Member

quetzalliwrites commented Nov 15, 2022

I just noticed we did not have branch protection and code owners requirement for review for `community branch. Solved

Yay!!!!!!!!!!

This is what I was trying to solve when I added the DNM label. Of course adding branch protection is the appropriate way to fix this so now that Lucas made that improvement, I have removed the DNM label 😂

@derberg
Copy link
Member

derberg commented Nov 15, 2022

You sure? https://deploy-preview-1071--asyncapi-website.netlify.app/community/events

Interesting, 30min ago I was getting an internal server error. Thanks!

The scope of review is design and text @derberg. The source of data is temporary for now... I had to use it just to have some output

The core of the issue is when you focus first on UI without real data you might design something that is not technically possible.

Examples:

the point is that whatever we approve here, you will anyway rework in the next PR. You know what I mean?

cuz some reviewers think the missing pages weren't cool

let's negotiate with these reviewers 😄 cause I bet that for the majority better is when PRs are super small 😄 (I bet $100 on it 😄 )

@AceTheCreator
Copy link
Member Author

@derberg @alequetzalli @akshatnema I think the best way forward is to eventually merge this PR and start tackling the above request one after the other with a smaller PR :)

Copy link
Member

derberg commented Nov 16, 2022

hehe, nice try ☺️
Problem is that if you merge, then how can we provide feedback, add comments, etc. PR way is the best.

I know you will hate me for this comment but really the best way forward is to split it into 3 PRs 😅

@quetzalliwrites
Copy link
Member

quetzalliwrites commented Nov 16, 2022

Agreed... @AceTheCreator, you're so sneaky, trying to merge PRs before they're ready 😂

Splitting this into smaller PRs (looks like Lukasz thinks 3 PRs are sufficient) is def the way to go 🙌🏽🙌🏽

I know it may seem "good enough" to use dummy data in the efforts toward getting this done faster, but like Lukasz notes above, you cannot know if your UI components will work with real-life data. And you shouldn't merge this to wait to find out how it works 😂

@quetzalliwrites
Copy link
Member

quetzalliwrites commented Nov 16, 2022

(1) Menu item bug

Hey @AceTheCreator, there is still a punctuation issue in the menu item. You should copy/paste the text I gave you to help you avoid further punctuation mistakes 😄✌🏽 (you can also show me directly which file this change should be made to and I can make that commit.. just like I said above, I couldn't figure out which file to make that change to)

Let's fix the menu item to read: Connect, share, and learn.

Screen Shot 2022-11-16 at 11 06 59 AM

Screen Shot 2022-11-16 at 11 07 16 AM

(2) odd spacing in mobile view

There is still an odd spacing issue in mobile.

Are you making sure to validate your changes in diff devices? Remember: you should always ensure your UI changes are responsive. You need to use a simulator or the default web browser tools for device validation.

Screen Shot 2022-11-16 at 11 11 45 AM

@AceTheCreator
Copy link
Member Author

I split this PR into smaller ones based on what @derberg and @alequetzalli suggested

Community landing page

#1119

Ambassador page

#1123

Events page

#1120

Contributors page

#1122

I believe it would be easier now to make review much more efficient :)

cc @derberg @alequetzalli @akshatnema @Barbanio

@derberg
Copy link
Member

derberg commented Feb 28, 2023

Closing this PR as there were separate PRs processed

@derberg derberg closed this Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants