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

Added Home Page Slider #199

Merged
merged 3 commits into from Jun 15, 2018
Merged

Added Home Page Slider #199

merged 3 commits into from Jun 15, 2018

Conversation

BaaniLeen
Copy link
Member

@BaaniLeen BaaniLeen commented Jun 1, 2018

Description

Added a slider on the Home Page using Angular carousel, and ngFor to ensure code modularity.

In order to add/remove/change an image, only the image path and description need to be added in the slides array in home.component.ts file, hereon.

Fixes #193

Time Taken :

11 hours (Implementing it + Incorporating Mentor suggestions+ Discussion)
Populating the slider with images in a modular format : 3 hours
Add a timer to automate changing of pictures after a fixed span of time : 3 hours
Add dots below, highlighted dot represents the progress number of the picture- 2 hours
Collecting pictures which can be used on Systers Open Source Home Page slider- 0.5 hours
Creating the Pr - 0.5 hour
Ensure mobile responsiveness - 2 hours.

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?

This is the demo of how it looks.
https://drive.google.com/file/d/1agRLN6y3VGXOXlu5wcZnk4jzGrEUGUb_/view?usp=sharing.

Mobile Responsiveness:
https://drive.google.com/file/d/1vS29F8yXYNdNDx7ir9gebBfA_Ro0O6NQ/view?usp=sharing

Checklist:

Delete irrelevant options.

  • 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

Code/Quality Assurance Only

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@ancyp
Copy link
Contributor

ancyp commented Jun 1, 2018

mention mobile testing!
Footer goes the full width of the page =>was Issue for this created?

@BaaniLeen
Copy link
Member Author

@ancyp Sure :)
I was waiting for the confirmation regarding which all tab headings need to be added in the header and then combine both the points and make a new issue. No worries. I will do it right away :)

@BaaniLeen
Copy link
Member Author

@ancyp Here is the link for the demo regarding mobile responsiveness of the Home Page Slider:
https://drive.google.com/file/d/1vS29F8yXYNdNDx7ir9gebBfA_Ro0O6NQ/view?usp=sharing :)

@ancyp
Copy link
Contributor

ancyp commented Jun 2, 2018

@BaaniLeen need not give demo link. I meant just a checklist option for saying - it has been tested on mobile. :)

@ancyp
Copy link
Contributor

ancyp commented Jun 2, 2018

screen shot 2018-06-01 at 5 24 09 pm

The whole Image should fit within the screen (currently have to scroll).
Also it would look nice if the image is fully fitting on the screen without left and right margins.

The text can go on a black or white background, shrinking image height by 25% as text over image is unreadable.

@janiceilene what do you think?

@janiceilene
Copy link
Member

I agree with the photo going the full width and definitely should be short enough that there's no need to scroll. Didn't we discuss removing those types of images for mobile versions, @BaaniLeen ?

Copy link
Member

@divyanshu-rawat divyanshu-rawat left a comment

Choose a reason for hiding this comment

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

Kindly see the comments.

@divyanshu-rawat
Copy link
Member

Situation gets worse when navigation-bar is opened.
screen shot 2018-06-03 at 11 38 48 pm

@BaaniLeen
Copy link
Member Author

@divyanshu-rawat I have already created a new issue for the responsiveness of the Navbar and the footer and am trying to fix that.
Can you please comment out the from the app.component.html tag and then look at the mobile responsiveness of the Home Page slider alone for this PR, as shown in the video attached?
Thanks :)

@divyanshu-rawat
Copy link
Member

@BaaniLeen I wil check and let you know 👍

@@ -1 +1,12 @@
<h1> HOME PAGE</h1>
<div class=" container box logos mx-auto d-block mt-5 mb-5">
Copy link
Contributor

Choose a reason for hiding this comment

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

extra space

Copy link
Member

@divyanshu-rawat divyanshu-rawat left a comment

Choose a reason for hiding this comment

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

LGTM! responsive without navbar and footer,
Although Navbar is already fixed in #206

@BaaniLeen
Copy link
Member Author

@ancyp Can you please approve and merge this PR. I have made the suggested changes. :)

@ancyp ancyp merged commit 4e38598 into systers:gsoc18-code Jun 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants