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

The Portrait Immersive Main Media Width Problem #2205

Merged
merged 6 commits into from
Dec 8, 2020
Merged

The Portrait Immersive Main Media Width Problem #2205

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8ea77fc
Don't choose immersive main media widths based on viewport
oliverlloyd Dec 7, 2020
8c99690
Use orientation portrait and vh to work out width
oliverlloyd Dec 7, 2020
6cf9201
Update src/web/components/Img.tsx
oliverlloyd Dec 8, 2020
6aaf1d5
Updated comment for 167vh approach
oliverlloyd Dec 8, 2020
28520b8
Reduce Chromatic check threashold for these image stories
oliverlloyd Dec 8, 2020
e39fa8a
Increase threshold for these images
oliverlloyd Dec 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion src/web/components/Img.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,17 @@ const buildSourcesString = (srcSets: SrcSetItem[]): string => {
return srcSets.map((srcSet) => `${srcSet.src} ${srcSet.width}w`).join(',');
};

/**
* mobile: 320
* mobileMedium: 375
* mobileLandscape: 480
Copy link
Contributor

Choose a reason for hiding this comment

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

how accurate is that, nowadays?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not very, the name is certainly confusing for me. We design to it though so it would be hard to remove.

Copy link
Contributor

Choose a reason for hiding this comment

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

The names are somewhat arbitrary. We need to specify breakpoints somewhere - we could probably name them 1-7 if we wanted to 🤷‍♂️.

* phablet: 660
* tablet: 740
* desktop: 980
* leftCol: 1140
* wide: 1300
*/

const buildSizesString = (role: RoleType, isMainMedia: boolean): string => {
switch (role) {
case 'inline':
Expand All @@ -57,8 +68,17 @@ const buildSizesString = (role: RoleType, isMainMedia: boolean): string => {
case 'thumbnail':
return '140px';
case 'immersive':
// Immersive MainMedia elements fill the height of the viewport, meaning
// on mobile devices even though the viewport width is small, we'll need
// a larger image to maintain quality. Deciding which width image to ask
// for is difficult because aspect ratios and orientations between
// different devices varies so the values given below are best estimates
// based on the most popular configurations
oliverlloyd marked this conversation as resolved.
Show resolved Hide resolved

// Immersive body images stretch the full viewport width below wide,
// but do not stretch beyond 1300px after that.
return isMainMedia
? '100vw'
? `(orientation: portrait) 167vh, 100vw`
: `(min-width: ${breakpoints.wide}px) 1300px, 100vw`;
case 'supporting':
return `(min-width: ${breakpoints.wide}px) 380px, 300px`;
Expand Down