✨ Add customization options to <amp-story-quiz>#26714
✨ Add customization options to <amp-story-quiz>#26714jackbsteinberg merged 7 commits intoampproject:masterfrom
Conversation
|
Hey @newmuis, these files were changed:
|
|
Quick follow-up, the color for the |
|
Can you add a list of each of the attributes/properties and their allowed values to the PR description? |
Updated |
gmajoulet
left a comment
There was a problem hiding this comment.
Bike-shedding: The fact that you can write your own HTML but not your own CSS (can only override a set of variables) is a pattern that no other AMP/STAMP components are using. I really think we should switch to a JSON configuration: "the DOM is generated for you but you can still tweak the design with CSS variables" is much easier to understand than simply ignoring publisher's CSS, and would be aligned with other AMP components or even amp-story-consent.
This could be a followup PR tho
| /* 224px = the width of a narrow phone (320px) - tap-safe margins (48px * 2) */ | ||
| min-width: 224px !important; | ||
| max-width: 400px !important; | ||
| --accent-color: #005AF0; |
There was a problem hiding this comment.
All these CSS variables are very broad and should be prefixed with something (quiz or reaction or..)
There was a problem hiding this comment.
What would it look like if we iterate to ship polls tomorrow then? Wouldn't we want a unified theming, so whoever implements quizzes also has polls support for free?
There was a problem hiding this comment.
Alternatively, the publisher won't necessarily want the same configuration for polls and quizzes, it might be strange to force one value that covers both.
A compromise I could strike is prefixing the more general CSS custom properties --reaction-, and having --quiz- and --poll- custom properties inherit from that, but have their own values in case the publisher wanted to override.
There was a problem hiding this comment.
Alternatively, the publisher won't necessarily want the same configuration for polls and quizzes, it might be strange to force one value that covers both.
From this PR, publishers will have to either set a global CSS variable that will apply to all quizzes, or target a specific quiz with its ID. So they can still target polls or quizzes with either their specific ID, or amp-story-quiz {/* variables*/} amp-story-poll {/* variables */}
I know I'm very annoying but I really think launching quizzes + polls + sliders at the same time is the way to go and would like the next iterations to be as easy and fast as possible, as it unlocks a lot of new business use cases for publishers to receive feedback.
There was a problem hiding this comment.
Ah, true! Common naming could make a lot of sense here
| max-width: 400px !important; | ||
| --accent-color: #005AF0; | ||
| --prompt-text-color: white; | ||
| --option-text-color: #5F6368; |
There was a problem hiding this comment.
Do we want to let publishers override the text color?
There was a problem hiding this comment.
In our UX talks we considered it as an adjustable UI element - we could decide otherwise though
I think a JSON config would be good, but I feel very strongly that we shouldn't launch an API that is not supported by AMP validation. So if you take that to be true, our choices are (a) block the quiz validation until the AMP validator supports validating JSON configurations, or (b) use an API that the validator can validate. Either way, definitely should happen outside of the scope of this PR. |
036827e to
b78bf73
Compare
* master: Launch `amp-next-page` v2 & clean up experiment (ampproject#27035) ✨Implement Display Locking on amp-accordion (ampproject#25867) 📖<amp-next-page> documentation and validation (ampproject#26841) Improve visibility event tests (ampproject#26847) amp-geo: Fall back to API for country (ampproject#26407) ✨ Add customization options to <amp-story-quiz> (ampproject#26714) navigation: Minor test improvements (ampproject#26926) ♻️ Alias video.fullscreen action for symmetry with interface names (ampproject#27017) ✨ Implements `amp-analytics` support for `amp-next-page` (ampproject#26451) ✨ amp-video-iframe integration: global jwplayer() singleton by default (ampproject#26969) Fix unit tests broken by ampproject#26687 (ampproject#27000) Filter redirect info from emails (ampproject#27012) 📖 Make amp-bind doc valid, fix amp-form stories filter (ampproject#27003) url-replacements: Minor test improvement (ampproject#26930) viewport: Minor test improvement (ampproject#26931) amp-consent fullscreen warning (ampproject#26467) dep-check: USE CAPS FOR IMPORTANCE (ampproject#26993) fix img url (ampproject#26987) # Conflicts: # extensions/amp-next-page/amp-next-page.md
Adds methods to customize the element via CSS custom properties for color customizations, or HTML attributes on the element for other customizations.
Additionally adds support for leaving off the prompt of a quiz, in the case where a publisher wants to present only the quiz options.
Attributes
theme: controls the theme of the quiz -light,darkchip-corner: controls the sharpness of the corners on the option chip -round,sharpchip-style: controls the UI style of the option chips -outline,shadowCustom Properties:
--accent-color: controls the accent color of the quiz - default#005AF0--prompt-text-color: controls the color of the prompt text - default#FFFFFF--option-text-color: controls the color of the option text - default#5F6368Adds work related to tracking issue #25615