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

fix(docs-infra): make header full-width and panels full-width on mobile screens #34188

Closed
wants to merge 1 commit into from

Conversation

@ajitsinghkaler
Copy link
Contributor

ajitsinghkaler commented Dec 2, 2019

Fixes #34163

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

In the current behavior, the width of the header is lesson events page in mobile screen and I made resources full screen on the mobile screen

Issue Number: #34163

What is the new behavior?

The header is full screen and resources are full screens on mobile.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@ajitsinghkaler ajitsinghkaler requested a review from angular/docs-infra as a code owner Dec 2, 2019
@googlebot googlebot added the cla: yes label Dec 2, 2019
@ajitsinghkaler

This comment has been minimized.

Copy link
Contributor Author

ajitsinghkaler commented Dec 2, 2019

@gkalpak can you please help me and tell me why the integration test is failing.

@ajitsinghkaler ajitsinghkaler force-pushed the ajitsinghkaler:scss-changes branch from 416c06e to 09ee065 Dec 2, 2019
@ngbot ngbot bot modified the milestone: needsTriage Dec 3, 2019
@gkalpak gkalpak requested a review from sjtrimble Dec 3, 2019
@gkalpak gkalpak added the aio: preview label Dec 3, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 3, 2019

Copy link
Member

gkalpak left a comment

Thx, @ajitsinghkaler! Can you, please, update the commit message to fix(docs-infra): ... and also add some more info on what exactly it is fixing (e.g. the events and resources pages)?

@@ -411,6 +411,7 @@ div[layout=row]{
background-color: lighten($blue, 10);
margin-top: 64px;
padding: 32px;
width:100%;

This comment has been minimized.

Copy link
@gkalpak

gkalpak Dec 3, 2019

Member

Please, put a space after :.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Dec 3, 2019

Member

Also, can you explain what difference does this make? .marketing-banner is only set on <header> (afaict), which is a block element by default. So, width, shouldn't make a difference. Did I miss something?

This comment has been minimized.

Copy link
@ajitsinghkaler

ajitsinghkaler Dec 3, 2019

Author Contributor

In the current scenario if the table in the body of the docs takes more than screen width the header does not take 100% of the screen. That is if the screen is horizontally scrollable on mobile screens then the header does not take full width making width:100% lets it take full width in those scenarios also. I've attached a screenshot for reference.

Angular - EVENTS (1)

This comment has been minimized.

Copy link
@gkalpak

gkalpak Dec 3, 2019

Member

I see. I don't think this is the right fix (because I still see it not taking up 100% of the scroll width.
I believe you need to set overflow: auto (or overflow-x: auto) to the <article> element there.

This comment has been minimized.

Copy link
@ajitsinghkaler

ajitsinghkaler Dec 4, 2019

Author Contributor

but I've checked it in the build the header is working correct I've attached screenshots and I'm sending you the link of the build page for your reference maybe you are talking about the table or body below the header.

Link: https://pr34188-09ee065.ngbuilds.io/events. Please use this link in the mobile or responsive web version.

Angular - EVENTS

This comment has been minimized.

Copy link
@ajitsinghkaler

ajitsinghkaler Dec 4, 2019

Author Contributor

Desktop screenshot (2)

This comment has been minimized.

Copy link
@gkalpak

gkalpak Dec 4, 2019

Member

Your change does fix the issue on some screen sizes, but not all. If the screen is smaller (e.g. in DevTools choose iPhone 5) there is still an empty space on the right of the header:

image

Do you have any objection against my suggestion in #34188 (comment)?

This comment has been minimized.

Copy link
@ajitsinghkaler

ajitsinghkaler Dec 5, 2019

Author Contributor

No I didn't have any objection it was just width:100% was working in my pc.

aio/src/styles/2-modules/_resources.scss Outdated Show resolved Hide resolved
@ajitsinghkaler ajitsinghkaler force-pushed the ajitsinghkaler:scss-changes branch from 09ee065 to 3eedf88 Dec 3, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 3, 2019

@ajitsinghkaler ajitsinghkaler requested a review from gkalpak Dec 3, 2019
@ajitsinghkaler

This comment has been minimized.

Copy link
Contributor Author

ajitsinghkaler commented Dec 4, 2019

@sjtrimble @gkalpak now you can have a look I attached screenshots for what changes does this PR make.

@ajitsinghkaler ajitsinghkaler changed the title docs: make header full-width and panels full-width on mobile screens fix(docs-infra): make header full-width and panels full-width on mobile screens Dec 4, 2019
@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Dec 4, 2019

Thx, @ajitsinghkaler. Can you also take care of the second part of #34188 (review)?

@ajitsinghkaler ajitsinghkaler force-pushed the ajitsinghkaler:scss-changes branch from 3eedf88 to b934e2c Dec 5, 2019
@ajitsinghkaler ajitsinghkaler requested a review from angular/fw-docs-marketing as a code owner Dec 5, 2019
@ajitsinghkaler

This comment has been minimized.

Copy link
Contributor Author

ajitsinghkaler commented Dec 5, 2019

@gkalpak I've integrated the changes you suggested and also changed the commit message please have a look

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 5, 2019

@gkalpak
gkalpak approved these changes Dec 5, 2019
aio/content/marketing/events.html Show resolved Hide resolved
aio/src/styles/1-layouts/_marketing-layout.scss Outdated Show resolved Hide resolved
…le screens

On events page the header was not able to take full width when body exceeds viewport width of the screen So made the below body go overflow-x auto and resources page was taking 80% of the width which is okay on desktop but on mobile it should take 100% width put a media quer for it.

Fixes #34163
@ajitsinghkaler ajitsinghkaler force-pushed the ajitsinghkaler:scss-changes branch from b934e2c to 9f2ae82 Dec 5, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 5, 2019

@ajitsinghkaler

This comment has been minimized.

Copy link
Contributor Author

ajitsinghkaler commented Dec 5, 2019

@gkalpak I've made the changes please approve the pull request.

@ajitsinghkaler ajitsinghkaler requested a review from gkalpak Dec 5, 2019
@gkalpak
gkalpak approved these changes Dec 5, 2019
@gkalpak gkalpak removed the request for review from sjtrimble Dec 5, 2019
@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Dec 5, 2019

merge-assistance: This is a docs-infra only change (it doesn't touch marketing content). My approval should be enough.

AndrewKushnir added a commit that referenced this pull request Dec 5, 2019
…le screens (#34188)

On events page the header was not able to take full width when body exceeds viewport width of the screen So made the below body go overflow-x auto and resources page was taking 80% of the width which is okay on desktop but on mobile it should take 100% width put a media quer for it.

Fixes #34163

PR Close #34188
@ajitsinghkaler ajitsinghkaler deleted the ajitsinghkaler:scss-changes branch Dec 5, 2019
josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 11, 2019
…le screens (angular#34188)

On events page the header was not able to take full width when body exceeds viewport width of the screen So made the below body go overflow-x auto and resources page was taking 80% of the width which is okay on desktop but on mobile it should take 100% width put a media quer for it.

Fixes angular#34163

PR Close angular#34188
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.