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

[UI:Mobile] Add mobile screen view for the grade-report page #5409

Merged
merged 4 commits into from May 27, 2020

Conversation

mukul-kmr-jha
Copy link
Contributor

@mukul-kmr-jha mukul-kmr-jha commented May 25, 2020

Please check if the PR fulfills these requirements:

  • The PR title and message follows our guidelines
  • Tests for the changes have been added/updated (if possible)
  • Documentation has been updated/added if relevant

What is the current behavior?

Currenlty the page layout breaks on smaller screen devices...
Screenshot 2020-05-25 at 7 51 32 PM

What is the new behavior?

Added responsiveness and now the page looks like this
image

Other information?

Involves a little code refactoring also as I scrapped out the table layout plus inline styling in the report page and created a different CSS file for styling.

Add smaller screens view

fix line break in buttons
@codecov
Copy link

codecov bot commented May 25, 2020

Codecov Report

Merging #5409 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5409      +/-   ##
============================================
- Coverage     20.87%   20.86%   -0.01%     
  Complexity     5945     5945              
============================================
  Files           147      147              
  Lines         19271    19272       +1     
============================================
  Hits           4022     4022              
- Misses        15249    15250       +1     
Flag Coverage Δ Complexity Δ
#autograder 10.75% <ø> (ø) 0.00 <ø> (ø)
#migrator 100.00% <ø> (ø) 0.00 <ø> (ø)
#php 20.52% <0.00%> (-0.01%) 5945.00 <0.00> (ø)
#python_submitty_utils 3.04% <ø> (ø) 0.00 <ø> (ø)

Copy link
Member

@shailpatels shailpatels left a comment

Choose a reason for hiding this comment

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

The mobile view should look similar to the desktop view so I think the border boxes should be removed. Take a look at how the course settings page looks similar to the desktop view but with a few small changes to fit on mobile displays. We probably don't need any border but you can use the <hr> delimiter instead

Add the line $this->core->getOutput()->enableMobileViewport(); to the report controller under showReportPage to enable media queries based on display size

A bigger issue is that we should be writing mobile-first CSS this means if we're adding new CSS the display page should be first programmed to render on mobile screens and media queries should be used after to target desktop and tablet views.
https://zellwk.com/blog/how-to-write-mobile-first-css/

Take a look at navigation.css and the navigation page to see how this done. On that note try and reuse CSS that is defined everywhere like in server.css before writing more.

site/app/templates/admin/Report.twig Outdated Show resolved Hide resolved
site/public/css/grade-report.css Outdated Show resolved Hide resolved
@mukul-kmr-jha
Copy link
Contributor Author

A bigger issue is that we should be writing mobile-first CSS this means if we're adding new CSS the display page should be first programmed to render on mobile screens and media queries should be used after to target desktop and tablet views.

Done for this PR.
I'm planning to use this suggestion for future PRs also as I move on to the next pages ( which will result in refactoring) because currently, we have targeted the desktop screen instead of using mobile-screen first strategy.
Thanks!

Copy link
Member

@shailpatels shailpatels left a comment

Choose a reason for hiding this comment

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

Looks good this should be ready to merge,
Here is the updated view
image

@bmcutler bmcutler merged commit 4d0e5f8 into Submitty:master May 27, 2020
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.

None yet

3 participants