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

UI:Added Benefit section to home page #245

Closed
wants to merge 6 commits into from
Closed

UI:Added Benefit section to home page #245

wants to merge 6 commits into from

Conversation

DhaneshShetty
Copy link

Description

Developed Benefit section UI of the Home page.

Fixes #211

Type of Change:

  • Code
  • User Interface

Code/Quality Assurance Only

  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
  • Any dependent changes have been merged

Code/Quality Assurance Only

  • My changes generate no new warnings

@welcome
Copy link

welcome bot commented May 16, 2021

Hello there!👋 Welcome to the project!💖

Thank you and congrats🎉 for opening your first pull request.✨ AnitaB.org is an inclusive community, committed to creating a safe and positive environment.🌸Please adhere to our Code of Conduct and Contribution Guidelines.🙌.We will get back to you as soon as we can.😄

Feel free to join us on AnitaB.org Open Source Zulip Community.💖 We have different streams for each active repository for discussions.✨ Hope you have a great time there!😄

@devkapilbansal devkapilbansal added the Status: Needs Review PR needs an additional review or a maintainer's review. label May 18, 2021
mtreacy002
mtreacy002 requested changes May 22, 2021
View reviewed changes
Copy link
Member

@mtreacy002 mtreacy002 left a comment

Choose a reason for hiding this comment

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

@DhaneshShetty , can you please resolve the conflict first? I will give my review after you've done that. Thanks 😉

@DhaneshShetty DhaneshShetty requested a review from mtreacy002 May 22, 2021 12:04
naveen8801
naveen8801 reviewed Jun 3, 2021
View reviewed changes
src/home/Benefits.jsx Outdated Show resolved Hide resolved
src/home/Benefits.jsx Outdated Show resolved Hide resolved
@mtreacy002
Copy link
Member

mtreacy002 commented Jun 17, 2021 •
edited
Loading

@DhaneshShetty , can you please give us update on this? @naveen8801 has given some feedback. Can you give your response? Thanks 😉. Also, if you still working on this, please resolve the conflict.

@mtreacy002 mtreacy002 added Category: User Interface Improvements or additions to design. Status: Changes Requested Changes are required to be done by the PR author. Type: Enhancement New feature or request. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Jun 17, 2021
@mtreacy002 mtreacy002 mentioned this pull request Jun 17, 2021
Merged
3 tasks
@DhaneshShetty
Copy link
Author

I will do the required changes soon.Sorry for the delay.

@mtreacy002 mtreacy002 added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Changes Requested Changes are required to be done by the PR author. labels Jun 21, 2021
mtreacy002
mtreacy002 requested changes Jun 21, 2021
View reviewed changes
Copy link
Member

@mtreacy002 mtreacy002 left a comment

Choose a reason for hiding this comment

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

Thank you, @DhaneshShetty for your work on this section. Please check my feedback below for some suggestions for improvement.

  • The balance between each row (image and description) and the left-center-right alignments seems to be different to the design that got approved.

    • from this pr

Screen Shot 2021-06-21 at 11 34 23 pm

* from the Figma file

Screen Shot 2021-06-21 at 11 39 52 pm

Will it be possible for you to adjust this to match the Figma file? Please let me know what you think. cc @Vuyanzi and @Rahulm2310

@mtreacy002 mtreacy002 added Status: Changes Requested Changes are required to be done by the PR author. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Jun 21, 2021
@mtreacy002 mtreacy002 requested review from Rahulm2310 and Vuyanzi June 21, 2021 13:44
@mtreacy002
Copy link
Member

@DhaneshShetty , as we receive no comments from the other leaders, I'm happy to accept it as it it for now and fix the content balance issue separately if need to. Can you please resolve the issue so I can merge it to develop branch? Thanks

@vj-codes
Copy link
Member

@DhaneshShetty can you please try to align the space alignment balance as suggested by @mtreacy002 ?
It's always better to have consistent spacing:)

@mtreacy002
Copy link
Member

Will close this PR since there's no activity

@mtreacy002 mtreacy002 closed this Nov 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@naveen8801 naveen8801 naveen8801 left review comments

@mtreacy002 mtreacy002 mtreacy002 requested changes

@Rahulm2310 Rahulm2310 Awaiting requested review from Rahulm2310

@Vuyanzi Vuyanzi Awaiting requested review from Vuyanzi

Assignees

@DhaneshShetty DhaneshShetty

Labels
Category: User Interface Improvements or additions to design. Status: Changes Requested Changes are required to be done by the PR author. Type: Enhancement New feature or request.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

UI: Develop Homepage - Benefits section
5 participants
@DhaneshShetty @mtreacy002 @vj-codes @naveen8801 @devkapilbansal