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

Prevent mobile views having elements that are being cut off. #3737

Merged
merged 1 commit into from
Feb 13, 2018

Conversation

prinzdezibel
Copy link
Contributor

@prinzdezibel prinzdezibel commented Feb 13, 2018

Closes issue #3396.

  • Retained behaviour: Uses 400px width for the action view where viewport width >= 400 px
  • Changed: Uses viewport width for action view when viewport width < 400px
  • Removed unnecessary media query from inline style (@media will not work there anyway)
-    "@media only screen and (max-width: 949px)": {	
-        width: "100vw"
-    },
  • Removed unnecessary width: "50%" , because width is calculated through flex-basis (has precedence)

  • Add new property viewportWidth to container, which serves as equivalent to a css media query . Moved it out to container, because it's environment dependent and windows is possibly not defined. Let me know if you think it makes sense to put it back to the component.

  • Remove unnecessary width rule from .admin-controls-content

@spencern
Copy link
Contributor

spencern commented Feb 13, 2018

@prinzdezibel
Gonna merge this since it has an approved PR.
One thing I would like to see going forward is that PRs which make changes or fix UI issues include screenshots of before and after. This will help us know what it now looks like and permit @rymorgan and @cassytaylor to get involved with design reviews without having to boot up the whole app

@spencern spencern changed the base branch from master to release-1.8.0 February 13, 2018 16:56
@spencern spencern merged commit c5b7326 into release-1.8.0 Feb 13, 2018
@spencern spencern deleted the fix-3396-prinzdezibel-fix-cut-off-mobile branch February 13, 2018 16:59
@brent-hoover
Copy link
Collaborator

@spencern Can we add that additional requirement to the Pull Request template doc?

@spencern
Copy link
Contributor

@zenweasel good idea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants