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

Resize app preview properly for tablet view on page load #34580

Merged
merged 1 commit into from
May 8, 2024

Conversation

orangejenny
Copy link
Contributor

@orangejenny orangejenny commented May 8, 2024

Product Description

https://dimagi.atlassian.net/browse/SAAS-15524

Technical Summary

Probably introduced by the requirejs migration. As explained in ticket:

This width is controlled via the .preview-tablet-mode .scrollable-container selector (here). This class is applied to the outer js-appmanager-preview div, but that doesn’t matter, because it’s outside the iframe, so the styles don’t get inherited. It needs to also be applied to the body tag within the iframe. This happens via the view:tablet handler here. This event does get triggered on page load, here. However, if web apps / app preview javascript hasn’t fully loaded at that point, the handler hasn’t been created yet, so preview-tablet-mode doesn't get applied to the iframe body, and the display is squashed.

I don't like that formplayer js now knows about data that's conceptually part of app manager, but I do like that this is simple. The best alternative is probably to have app preview to signal to app manager when it's finished loading, but the app manager / app preview relationship is currently structured so that app manager sends instructions to app preview, not the other way around.

Safety Assurance

Safety story

Risk is pretty minor here, the change is quite targeted. Tested locally.

Automated test coverage

No

QA Plan

Not requesting QA

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@orangejenny orangejenny added the product/all-users-all-environments Change impacts all users on all environments label May 8, 2024
@orangejenny orangejenny requested review from biyeun, a team, MartinRiese and minhaminha and removed request for a team May 8, 2024 14:55
@orangejenny orangejenny requested a review from MartinRiese as a code owner May 8, 2024 14:55
@orangejenny orangejenny requested a review from stephherbers May 8, 2024 14:55
@dimagimon dimagimon added the Risk: Medium Change affects files that have been flagged as medium risk. label May 8, 2024
@orangejenny orangejenny merged commit f32611e into master May 8, 2024
12 checks passed
@orangejenny orangejenny deleted the jls/init-app-preview-tablet branch May 8, 2024 15:44
@kcowger kcowger added this to the test milestone Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/all-users-all-environments Change impacts all users on all environments Risk: Medium Change affects files that have been flagged as medium risk.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants