Skip to content

Fixed - [Bug] Foundation upgrade broke lay-out#189

Merged
canihavesomecoffee merged 4 commits into
CCExtractor:masterfrom
sanchaymittal:master
Feb 23, 2018
Merged

Fixed - [Bug] Foundation upgrade broke lay-out#189
canihavesomecoffee merged 4 commits into
CCExtractor:masterfrom
sanchaymittal:master

Conversation

@sanchaymittal

Copy link
Copy Markdown
Contributor

Please prefix your pull request with one of the following: [FEATURE] [FIX] [IMPROVEMENT].

In raising this pull request, I confirm the following (please check boxes):

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

My familiarity with the project is as follows (check one):

  • I have never used the project.
  • I have used the project briefly.
  • I have used the project extensively, but have not contributed previously.
  • I am an active contributor to the project.

Fixed the broken layout, working smoothly in both desktop and mobile layout.

@codecov

codecov Bot commented Feb 17, 2018

Copy link
Copy Markdown

Codecov Report

Merging #189 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #189   +/-   ##
======================================
  Coverage    1.32%   1.32%           
======================================
  Files          34      34           
  Lines        2789    2789           
  Branches      329     329           
======================================
  Hits           37      37           
  Misses       2751    2751           
  Partials        1       1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0e9977...8f06613. Read the comment docs.

@sanchaymittal

Copy link
Copy Markdown
Contributor Author

pre fix screenshots

screenshot from 2018-02-17 22-32-40
screenshot from 2018-02-17 22-32-06

post fix screenshots

screenshot from 2018-02-17 22-35-40
screenshot from 2018-02-17 22-35-25

@satyammittal

Copy link
Copy Markdown
Member

@sanchaymittal Please update your branch with master.

Comment thread static/css/app.css Outdated
summary {
display: block;
padding-bottom: 0.5%;
padding-inline-start: 1.5%;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you just want to fix the footer, why did you put it in here? Now this will apply to all those elements as well. That makes no sense.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can use a separate id or class for this but the changes now doesn't affect others because of their orientation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still, I'd like to see it in a separate section.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@canihavesomecoffee but it's better this way as it aligned with symmetry with others

Comment thread static/css/app.css

.flow {
display: flow-root;
padding-bottom: 50px;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this padding necessary?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Padding is required at the end of the page for mobile and other scroll screens.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@satyammittal

Copy link
Copy Markdown
Member

@sanchaymittal Please check the fix on chrome, It looks like
screenshot from 2018-02-21 18-28-23
and One more change, don't add unnecessary tab /+ spaces like you added in 340 line.

@sanchaymittal

Copy link
Copy Markdown
Contributor Author

In Chrome, I think it's perfectly ok
capture1
capture2
and i haven't noticed the extra tab
Thanks for that 🙂

@satyammittal

Copy link
Copy Markdown
Member

@sanchaymittal Great, I updated my google chrome to 64.0, Yeah it looks fine in both windows and linux.

Comment thread static/css/app.css
display: block;
padding-bottom: 0.5%;
padding-inline-start: 1.5%;
position:fixed;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So any menu element or any other will be fixed to the bottom? I'd still like to see a separate change for footer elements only.

@canihavesomecoffee canihavesomecoffee merged commit 1978060 into CCExtractor:master Feb 23, 2018
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.

3 participants