-
Notifications
You must be signed in to change notification settings - Fork 221
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 for full viewport of 375px for potrait images #5610
Conversation
…wport-for-375px-images
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Tested locally and appears to be working as expected.
@@ -35,6 +35,7 @@ const ArticleFigure = ({ | |||
}) => { | |||
const imageSpan = { | |||
default: '6', | |||
group1: '6', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Groups will inherit the default
value, so setting group1
to the same value is unnecessary. This change works, because portrait images have a different default value. We should probably apply this rule within the control block on line 47 instead. E.G.:
if (height > width) {
ParentWrapper = NestedGridParentSmall;
ChildWrapper = NestedGridItemChildSmall;
imageSpan.default = '4';
imageSpan.group1 = '6';
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion. Thanks @simonsinclair
…wport-for-375px-images
…bc/simorgh into full-viewport-for-375px-images
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
May be we should merge it and when on test should look on a real device. |
Resolves #5525
Overall change:
N/B: This is for just gel group1, portrait Images will still preserve their default grid span of 4 ie ( > 375px)
Code changes:
- Adds grid image span for gel group1 ( ~ 375px)
Testing:
CYPRESS_APP_ENV=local CYPRESS_SMOKE=false npm run test:e2e:interactive
)