-
Notifications
You must be signed in to change notification settings - Fork 212
[MWPW-182533], [MWPW-183428] - hero marquee japan cta spacing fix #5248
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
base: stage
Are you sure you want to change the base?
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch.
Commits
|
mokimo
left a comment
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.
I'm usually not a big fan of language specific customizations, because that sounds like a rabbithole that can never be satisfied, but we also don't have any good strategy for it either.
overmyheadandbody
left a comment
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.
This essentially reverts #4714 that was handling two issues on Firefox. What changed in the meantime, making those fixes redundant? I suggest you have a look through the original stories, MWPW-177979 & MWPW-177321 to ensure the functionality on Firefox is still intact. It would also be advisable to reach out to the initial committer to see if they have additional insights.
It reverts part of that PR , the one of which had to do with the buttons in general MWPW-177979(no mention of FF in the ticket) and the other it doesn't touch regarding FF MWPW-177321. After making these changes I see nothing wrong on Chrome or FF for buttons regarding spacing. I don't know what the origin of the original problem was, maybe it was also bad authoring that was tried to be fixed with code but spacing looks good on those browsers using just the gap for spacing rather then specific top/left margin calculations which were probably also wrong because they result in spacing of 19px which seems very odd, it could be also that some other PR after this fixed the original problem better than this PR and that now the issue is not visible when removing this part of the code, also looking at the screenshots of the original problem we can see that the space between the buttons was very big and the current solution is no where near that and has 24px which is the gap initially applied and seems very appropriate, more than these calculations. Regarding reaching out to the original committer that would prove impossible since he is no longer active in Adobe from what I can see. |
overmyheadandbody
left a comment
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.
I restored the page from Aug 6th, the last version before the bug was opened on Aug 11th and I can't reproduce the original bug either. You can find that here. I agree the original solution was less than ideal, but I'm also reluctant to remove it without some additional checks. I'll reach out with some additional steps to ensure we're not causing regressions. Approving for now so testing can start.
|
Reminder to set the |
NadiiaSokolova
left a comment
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.
Verified. Ready for Stage.
Testing details: MWPW-182533, MWPW-183428
This PR resolves hero marquee spacing issues for CTA's on JP pages.
Resolves: MWPW-182533, MWPW-183428
Test URLs:
CC1 (Previous ticket that implemented these changes) Test URLs:
CC2 (Mobile issue) Test URLs:
CC3 (1221px width resolution issue) Test URLs:
Events test URLs: