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

Finalise UI for About/Contribute/Teachers Page #1443

Merged
merged 9 commits into from
Jul 3, 2020
Merged

Finalise UI for About/Contribute/Teachers Page #1443

merged 9 commits into from
Jul 3, 2020

Conversation

aayushgupta05
Copy link
Member

@aayushgupta05 aayushgupta05 commented May 21, 2020 •
edited
Loading

Describe the changes you have made in this PR -

Made About/Contribute page fully responsive across all screen sizes and also refactored their code.

Video Recording -

https://drive.google.com/file/d/1yHszGq66QyrffuoNSxxDdnKv56KO3jnd/view

Teachers Page:
CircuitVerse - Teachers

codeclimate[bot]
codeclimate bot reviewed May 21, 2020
View reviewed changes
app/assets/stylesheets/about.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/about.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/layout.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/layout.scss Show resolved Hide resolved
app/assets/stylesheets/layout.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/layout.scss Show resolved Hide resolved
app/assets/stylesheets/layout.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/layout.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/layout.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/oldStyles.scss Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented May 21, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.6%) to 82.449% when pulling 533b5aa on aayush-05:finaliseAboutContributePage into 18f6c8c on CircuitVerse:master.

codeclimate[bot]
codeclimate bot reviewed May 21, 2020
View reviewed changes
app/assets/stylesheets/oldStyles.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/oldStyles.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/oldStyles.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/oldStyles.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/oldStyles.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/oldStyles.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/oldStyles.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/oldStyles.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/oldStyles.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/oldStyles.scss Outdated Show resolved Hide resolved
@aayushgupta05
Copy link
Member Author

@nitin10s @tachyons have a look at this; let me know if any screenshots required. I have added a video recording because showing those many screen sizes for each part would end up in a lot of screenshots.
Also @tachyons can I ignore the codeclimate issues for oldStyles.scss, I have made this file as a compilation of extra/irrelevant styles and it will simply be discarded when all the code would be refactored ?

nitin10s
nitin10s requested changes May 21, 2020
View reviewed changes
Copy link
Member

@nitin10s nitin10s left a comment

Choose a reason for hiding this comment

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

Looks good to me

Just Become a contributor button would redirect the user to the Contribute Page. The Contribute page will be removed from the navbar.

app/views/about/index.html.erb Outdated Show resolved Hide resolved
@aayushgupta05
Copy link
Member Author

@tachyons I have fixed all the codeclimate issues; but idk why the coverage is decreased

@tachyons
Copy link
Member

Not a blocker for merging

@aayushgupta05
Copy link
Member Author

Okay, then please review this one and merge

@aayushgupta05
Copy link
Member Author

@tachyons @nitin10s I had added refactored Teachers Page in this PR as well. Please review and let me know if any changes required.
(@nitin10s I have added shadow to the images on Teachers Page rather than a border, let me know if its fine)

@nitin10s
Copy link
Member

  1. On clicking the images, do they expand?
  2. The sub-heading "CircuitVerse has been designed to be very easy to use in class. The platform has features to assist teachers in class and assignments." looks bolder than it is currently, please make the appropriate changes.

@aayushgupta05
Copy link
Member Author

  1. No, the size of image doesn't change. On hover, the amount of shadow doubles.
  2. I haven't changed the font-weight or size for that. May be it is looking like that in the gif; refer this
    image

@nitin10s
Copy link
Member

Shadows are provided when we want to hint user that the item is clickable. I agree this looks better than the outlines but it gives our wrong cues.

@aayushgupta05
Copy link
Member Author

Ah, okay. Shall I revert them back to borders then ?

@nitin10s
Copy link
Member

Yeah

@aayushgupta05
Copy link
Member Author

Done @nitin10s
image

@aayushgupta05 aayushgupta05 mentioned this pull request May 25, 2020
Merged
nitin10s
nitin10s reviewed May 25, 2020
View reviewed changes
Copy link
Member

@nitin10s nitin10s left a comment

Choose a reason for hiding this comment

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

LGTM, @satu0king please check.

tachyons
tachyons reviewed May 26, 2020
View reviewed changes
app/assets/stylesheets/logixRelease.scss
@@ -1,3 +1,7 @@
body {
padding-top: 90px;
Copy link
Member

Choose a reason for hiding this comment

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

Curious Why ?

Copy link
Member Author

Choose a reason for hiding this comment

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

So that the top content doesn't overlap with the navbar

tachyons
tachyons reviewed May 26, 2020
View reviewed changes
app/assets/stylesheets/utilities.scss Outdated
@@ -0,0 +1,51 @@
//buttons
.button-primary {
Copy link
Member

Choose a reason for hiding this comment

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

Are you overriding bootstrap class here ?

Copy link
Member Author

@aayushgupta05 aayushgupta05 May 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

No, the bootstrap one is btn-primary. It is the class I made for our custom buttons; so that then it is just adding this classname for all other pages without having additional styles

Copy link
Member

Choose a reason for hiding this comment

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

may be components.scss ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@codeclimate
Copy link

codeclimate bot commented May 28, 2020

Code Climate has analyzed commit 533b5aa and detected 0 issues on this pull request.

View more on Code Climate.

@aayushgupta05 aayushgupta05 changed the title Finalise UI for About/Contribute Page Finalise UI for About/Contribute/Teachers Page May 30, 2020
nitin10s
nitin10s approved these changes Jun 3, 2020
View reviewed changes
Copy link
Member

@nitin10s nitin10s left a comment

Choose a reason for hiding this comment

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

This looks good to me @satu0king @tachyons

satu0king
satu0king approved these changes Jun 7, 2020
View reviewed changes
Copy link
Member

@satu0king satu0king left a comment

Choose a reason for hiding this comment

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

Looks great to me. @tachyons we can merge this.

This was referenced Jun 20, 2020
Merged
Merged
@satu0king satu0king merged commit 6065477 into CircuitVerse:master Jul 3, 2020
@aayushgupta05 aayushgupta05 deleted the finaliseAboutContributePage branch July 3, 2020 18:07
@animesh-007 animesh-007 mentioned this pull request Jul 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeclimate CodeClimate codeclimate left review comments

@satu0king satu0king satu0king approved these changes

@tachyons tachyons tachyons approved these changes

@nitin10s nitin10s nitin10s approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aayushgupta05 @coveralls @tachyons @nitin10s @satu0king