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

Widen dash container: changing mast head and content area max widths #10866

Merged
merged 2 commits into from
Dec 11, 2018

Conversation

jeffgolenski
Copy link
Member

@jeffgolenski jeffgolenski commented Dec 7, 2018

Widen dash container: changing mast head and content area max widths to
match those of calypso.

Fixes #9298

Before:
screen shot 2018-12-06 at 7 00 57 pm

After:
screen shot 2018-12-06 at 6 56 25 pm

Testing instructions:

  1. Run this branch
  2. Visit jetpack dashboard and settings areas
  3. Enjoy a wider experience on large screens

Proposed changelog entry for your changes:

"We've made the Jetpack dashboard wider on large screens for a better experience"

Widen dash container: changing mast head and content area max widths to
match those of calypso.
@jeffgolenski jeffgolenski self-assigned this Dec 7, 2018
@jeffgolenski jeffgolenski requested review from joanrho, keoshi, jeherve, dereksmart and a team December 7, 2018 00:01
@jetpackbot
Copy link

jetpackbot commented Dec 7, 2018

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: January 10, 2019.
Scheduled code freeze: January 3, 2019

Generated by 🚫 dangerJS

@jeffgolenski jeffgolenski added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Dec 7, 2018
@jeherve jeherve added Admin Page React-powered dashboard under the Jetpack menu [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Status] Needs Design Review Design has been added. Needs a review! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Dec 7, 2018
jeherve
jeherve previously requested changes Dec 7, 2018
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

It seems we lost some of the extra space in the header on smaller widths:

screenshot 2018-12-07 at 12 22 03

vs. in the latest stable:

image

The problem is the same with the old modules' list (wp-admin/admin.php?page=jetpack_modules);

screenshot 2018-12-07 at 12 29 46

max-width of container: adjusting for medium screens
@jeffgolenski
Copy link
Member Author

@jeherve Great find! I've made an adjustment and now the container has plenty of padding no matter the screen size.

Video attached: https://cloudup.com/cp3Q8rGuzJy

Copy link
Contributor

@keoshi keoshi 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 in my testing!

One nitpick is with the illustrations in the promo cards that have .jp-apps-card__top img { max-width: 40%; padding-top: 10px; }. That max-width paired with the wider wrapper makes these images take a lot of vertical space.

@keoshi keoshi added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Status] Needs Design Review Design has been added. Needs a review! labels Dec 10, 2018
@keoshi
Copy link
Contributor

keoshi commented Dec 10, 2018

@jeffgolenski One more thing: can we use the default breakpoints instead of fixed values here?

$breakpoints:(
phone: 320px,
large-phone: 530px,
phablet: 600px,
tablet: 782px,
desktop: 900px,
large-desktop: 1147px,
);

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Dec 10, 2018
@jeffgolenski
Copy link
Member Author

@keoshi I opted to create a specific breakpoint here because that's where it was needed. With the wp-admin sidebar, the 1250px was needed. I didn't think it warranted creating a new breakpoint in the scss though, since its only used once. 1147px was too small to fix the issue.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Dec 11, 2018
@jeherve jeherve dismissed their stale review December 11, 2018 11:51

Changes have been made

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Looking good now. Merging.

@ghost ghost removed [Status] Needs Changelog [Status] Ready to Merge Go ahead, you can push that green button! labels Dec 11, 2018
@jeherve jeherve deleted the enhancement/widen-dash-container branch December 11, 2018 11:52
jeherve added a commit that referenced this pull request Dec 19, 2018
jeherve added a commit that referenced this pull request Jan 3, 2019
jeherve added a commit that referenced this pull request Jan 3, 2019
* Add first version of the Changelog and testing list for 6.9

* Changelog: add #10710

* changelog: add #10538

* changelog: add #10741

* changelog: add #10749

* changelog: add #10664

* changelog: add #10224

* changelog: add #10788

* Changelog: add #10560

* Chanegelog: add #10812

* changelog: add #10556

* Changelog: add #10668

* Changelog: add #10846

* Changelog: add #10947

* Changelog: add #10962

* Changelog: add #10956

* Changelog: add #10940

* Changelog: add #10934

* Changelog: add #10912

* changelog: add #10866

* changelog: add #10924

* Changelog: add #10936

* Changelog: add #10833

* changelog: add #10867

* Changelog: add #10960

* Changelog: add #10888

* changelog: add #10840

* changelog: add #10972

* Changelog: add #10979

* changelog: add #10909

* Changelog: add #10958

* Changelog: add #10981

* Changelog: add #10564

* Changelog: add #10809

* Changelog: add #10982

* Changelog: add #10706

* Changelog: add #10978

* Changelog: add #10132

* Changelog: add #11022

* Changelog: add #11024

* Changelog: add #10875

* Changelog: add #11030

* Changelog: add #11053

* Changelog: add #10880

* Changelog: add #9359

* Changelog: add #11037

* Update block list

* Changelog: add #11060

* Changelog: add #10755

* changelog: add #11000

* Changelog: add #10786

* Changelog: add #10945

* Changelog: add #10597
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin Page React-powered dashboard under the Jetpack menu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants