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

Learning mode - Change all question styles for quiz page #7124

Merged

Conversation

Imran92
Copy link
Contributor

@Imran92 Imran92 commented Aug 24, 2023

Resolves #7070

Stacked on #7106

This PR contains the styles quiz 'taking' page. As we're working on the LM designs of quiz, not every change of design is applied in the editor, as many CSS styles are applied at runtime overriding the current theme's styles for LM. But the editor styles have been updated too for most font stylings and spacings.

We'll handle the quiz 'submitted' page in another PR.

Proposed Changes

  • Makes all the quiz, question and answer related blocks accept styles via theme.json and variations
  • Adds styles for File Upload input
  • Changes Checkbox styles
  • Changes Radio button styles
  • Adds code to allow adding CSS inside tinyMCE editor's content area inside iframe
  • The tinyMCE version Wordpress provides (4.6.2) doesn't have placeholder (starts from 5.2), so we've implemented that with custom js.
  • Overall updates all the styles for each question to match the styles as closely as possible for all variations of Course theme and other themes. Updated Non-LM styles so that it doesn't look broken either.

Testing Instructions

There are many combinations here. 7 questions (With and without input) * 4 variations (For course theme only) * 2 states (LM and Non - LM) * different screen sizes. All the states are not matched pixel perfectly, we tried to utilize existing spacings and font sizes wherever possible. So it shouldn't look too much different overall.

  1. Check out this branch of Course theme Update stylings for quiz, questions and answers themes#7331
  2. Check out this branch of Pro https://github.com/Automattic/sensei-pro/pull/2403
  3. Enable course theme
  4. Create a course with lessons
  5. Have at least one lesson with a quiz that has all the questions in it
  6. Make sure Learning Mode is enabled for that course
  7. Now check that quiz as a student
  8. Check in mobile screen size as well
  9. Change the theme variation to gold, dark and blue and check the styles for them too.
  10. Now switch to another block theme and check
  11. Switch to a non-block theme and check
  12. Check the editor and make sure it doesn't look broken.
  13. For all themes and variations, also check with LM disabled to make sure non-LM style isn't broken

image

image

Pre-Merge Checklist

  • PR title and description contain sufficient detail and accurately describe the changes
  • Acceptance criteria is met
  • Decisions are publicly documented
  • Adheres to coding standards (PHP, JavaScript, CSS, HTML)
  • All strings are translatable (without concatenation, handles plurals)
  • Follows our naming conventions (P6rkRX-4oA-p2)
  • Hooks (p6rkRX-1uS-p2) and functions are documented
  • New UIs are responsive and use a mobile-first approach
  • New UIs match the designs
  • Different user privileges (admin, teacher, subscriber) are tested as appropriate
  • Code is tested on the minimum supported PHP and WordPress versions
  • User interface changes have been tested on the latest versions of Chrome, Firefox and Safari
  • "Needs Documentation" label is added if this change requires updates to documentation
  • Known issues are created as new GitHub issues

Imran92 and others added 30 commits August 18, 2023 14:12
No theme styles will work otherwise
So that latest block options become available
Copy link
Collaborator

@donnapep donnapep left a comment

Choose a reason for hiding this comment

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

I think we can go ahead and merge this one if the file upload changes look OK.

@Imran92
Copy link
Contributor Author

Imran92 commented Sep 1, 2023

@Imran92 @merkushin I've made some changes to how file upload works. Basically, I'm hiding the input and wrapping it inside a label element that, when clicked, triggers the file upload process. This makes it easy to add whatever HTML we want it inside it and style it so that it uses the same classes as buttons do.

Haven't tested it yet, but ❤️ ed the approach of hiding it and using separate element to display and style as we wish.

Had to take a kind of similar approach with the radio and checkboxes to style them properly with ease. Looks like this has to be the de-facto way of styling browser native elements like these for frontend (and also editor in some cases) :p

@Imran92
Copy link
Contributor Author

Imran92 commented Sep 4, 2023

The file upload button works nicely as expected 💯 Just noticed some minor spacing differences, for example, in gold variation, the button height is 50px instead of 56px like design. But as we've discussed, it's better not to try and match 100% of design in this case as they'll have to definitely go inside theme.json's CSS which may become unmanageable with too many variation specific designs.

Just pushed a small PHPCS fix after the approval here 77a43eb.

As this is stacked on the footer branch ( #7106), should we merge that one to the feature branch first? Otherwise that PR may become a bit too big to check.

@donnapep
Copy link
Collaborator

donnapep commented Sep 5, 2023

Just noticed some minor spacing differences, for example, in gold variation, the button height is 50px instead of 56px like design.

Yup, this is how all buttons look for the Gold variation right now. I think it's more important to be consistent with existing button styles elsewhere (e.g. Complete Lesson, Take Quiz). If we get some feedback about this, we should be sure to update all buttons and not just the file upload button. 🙂

@donnapep
Copy link
Collaborator

donnapep commented Sep 5, 2023

As this is stacked on the footer branch ( #7106), should we merge that one to the feature branch first? Otherwise that PR may become a bit too big to check.

Sure, no problem. The footer PR has a bit of feedback that it would be nice to address prior to merge, but if neither of us can get to it quickly, we can create a new issue for that.

@Imran92
Copy link
Contributor Author

Imran92 commented Sep 6, 2023

Sure, no problem. The footer PR has a bit of feedback that it would be nice to address prior to merge, but if neither of us can get to it quickly, we can create a new issue for that.

Thanks @donnapep ! I've checked the footer PR too and only the alignment issue was outstanding. Alignment issue is fixed in this PR for both the questions and the footer ☺️

donnapep
donnapep previously approved these changes Sep 6, 2023
Base automatically changed from add/footer-style-update-lm-quiz to feature/frontend-improvements September 7, 2023 07:37
@Imran92 Imran92 dismissed donnapep’s stale review September 7, 2023 07:37

The base branch was changed.

@Imran92
Copy link
Contributor Author

Imran92 commented Sep 7, 2023

Hi @donnapep 👋 Sorry the review got automatically dismissed when I merged the footer branch to the feature branch, as the target branch got changed 😢 As it already got dismissed, I took the opportunity to fix some psalm issues as well. Can you kindly approve again? Thank you!

@donnapep donnapep merged commit 327c9e9 into feature/frontend-improvements Sep 7, 2023
21 of 23 checks passed
@donnapep donnapep deleted the add/change-all-question-styles branch September 7, 2023 15:39
@Imran92 Imran92 restored the add/change-all-question-styles branch September 8, 2023 01:50
@Imran92 Imran92 deleted the add/change-all-question-styles branch September 8, 2023 01:52
@donnapep donnapep linked an issue Sep 8, 2023 that may be closed by this pull request
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.

Update question styles in Learning Mode
3 participants