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

Responsive design for Donut #641

Merged
merged 42 commits into from
Oct 29, 2020

Conversation

lazycipher
Copy link

@lazycipher lazycipher commented Oct 5, 2020

This PR will solve the responsiveness of Donut for mobile.

@lazycipher
Copy link
Author

Homepage gif:

Donut

@lazycipher lazycipher marked this pull request as ready for review October 5, 2020 16:03
@vaibhavdaren
Copy link
Member

Nice progress, Looking forward to the completed pages 👍

@lazycipher
Copy link
Author

Responsive Navbar on the dashboard.
Backdrop yet to be added.
Donut (2)

Comment on lines +45 to +48
<div className="container-wrapper">
<div className="section-home">
<div className="welcome-text">
<div className="logo">
Copy link
Member

Choose a reason for hiding this comment

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

add all these classes into 1 div

Comment on lines 49 to 62
<DonutTitle />
</div>
<div className="tagline">
<h1>One place for meeting everyone.</h1>
</div>
<div className="description">
<p>
An Open Source Social networking bridge between Developers,
Organisations and Open Source aspirants
</p>
</div>
<div class="login-btn">
<a href="#login">
Sign In
Copy link
Member

Choose a reason for hiding this comment

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

find the proper way to do the same cc @ksraj123

Copy link
Author

Choose a reason for hiding this comment

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

@ksraj123, Whenever you're free, please let me know about how to proceed with this.

Copy link
Contributor

@ksraj123 ksraj123 Oct 12, 2020

Choose a reason for hiding this comment

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

using anchor tags for links which do not take you to a different website is not a good practice, for the use case here I think you want the user to be redirected to a different section of the page when he clicks on Sign In, for that you many use react-router-hash-link Also please make sure that the scrolling is smooth

Apart from this, Please try to avoid unnecessarily nesting of HTML elements verbose markup should be avoided, provide meaningful names to classes look into BEM naming convention, these suggestions are applicable for not just this piece of code but across the project.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, Sure thing! I'll follow this up and push the updated code.
But, I'm having issues in understanding how would I replace all divs with a single one?

Copy link
Member

@vaibhavdaren vaibhavdaren left a comment

Choose a reason for hiding this comment

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

Please catchup with @Rupeshiya @ksraj123 @AuraOfDivinity

@lazycipher
Copy link
Author

lazycipher commented Oct 11, 2020

The dashboard looks like this on Mobile Device as of now.

Donut (3)

Copy link
Member

@aayushgupta05 aayushgupta05 left a comment

Choose a reason for hiding this comment

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

I don't have Donut setup on local; so hadn't looked at how everything fits together and usability. Rather did a code review.

Also, on a sidenote: Why are we not following bootstrap grid system more religiously but rather with a combination of flexboxes? Why do the hardwork? :p

}

@media (min-width: 320px) and (max-width: 1024px){
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can extract these breakpoints too into variables

Copy link
Author

Choose a reason for hiding this comment

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

Alright! Sounds good actually! Will have to use mixins!

src/user/Activity/activityTimeline.scss Show resolved Hide resolved
src/user/Activity/Activity.js Outdated Show resolved Hide resolved
src/user/Activity/activityTimeline.scss Show resolved Hide resolved
src/user/auth/login-form/login-form.scss Show resolved Hide resolved
src/user/organization/organization.js Show resolved Hide resolved
src/user/organization/organization.js Outdated Show resolved Hide resolved
src/user/projects/projects.scss Show resolved Hide resolved
@Rupeshiya
Copy link
Member

@jaskiratsingh2000 Did you suggested for round buttons again? Because we already converted the whole platform buttons to rectangular shape earlier?
#inconsistencies

@lazycipher
Copy link
Author

lazycipher commented Oct 22, 2020

@Rupeshiya, The design on Figma had this round buttons so I did that!
I'll rectify if that's the case!

@jaskiratsingh2000
Copy link
Member

jaskiratsingh2000 commented Oct 22, 2020 via email

Copy link
Member

@Rupeshiya Rupeshiya left a comment

Choose a reason for hiding this comment

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

Nice work @lazycipher !
Please have a look at the comments

src/user/Activity/Activity.js Outdated Show resolved Hide resolved
src/user/Activity/Users.js Outdated Show resolved Hide resolved
Copy link
Member

@Rupeshiya Rupeshiya left a comment

Choose a reason for hiding this comment

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

@lazycipher Looks good, but once discuss that implementation of responsiveness for different views using third-party.
If possible I would suggest using a media query for that. (Check Navigation component)
Thanks

@lazycipher
Copy link
Author

@lazycipher Looks good, but once discuss that implementation of responsiveness for different views using third-party.
If possible I would suggest using a media query for that. (Check Navigation component)
Thanks

@Rupeshiya Thanks for reviewing. I'll use media queries in Navigation and will push code.

Copy link
Member

@Rupeshiya Rupeshiya left a comment

Choose a reason for hiding this comment

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

Nice work lazyCipher!!
Looks fine to me now.
Also, please try to reduce space in between tabs in mobile view
image

Make modals responsive too:
image

@ksraj123 If any improvement you want, please mention.

@Rupeshiya
Copy link
Member

Rupeshiya commented Oct 28, 2020

@jaskiratsingh2000 You can have a look the changes he made in the deploy version.
Here is the link: https://deploy-preview-641--friendly-sinoussi-3b191b.netlify.app/
Email: test3@gmail.com
Pass: abc123

cc @vaibhavdaren @AuraOfDivinity

@lazycipher
Copy link
Author

@Rupeshiya, Modals are responsive already!
The thing is that when toggling between multiple resolutions on the browser, it shows such anomaly sometimes.
Else everything's fine.

@lazycipher
Copy link
Author

@Rupeshiya, may we move ahead and merge this?

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.

None yet

6 participants