Enable User Brew theme selection #3321
Conversation
This has been implemented three different ways to allow for comparison and discussion - /api/css/:id : This returns the style frontmatter of the referenced document as a text/css document. /api/theme/:id : This returns an object with the reference'd object's theme and style frontmatter. /api/csstheme/:id : This returns the stylye frontmatter of the referenced document as a text/css document and adds the theme as an @import ( if not using the legacy renderer )
This adds a comment/field ( depending on endpoint ) that reports the name of the Brew being used as a theming source.
This consolidates the style/theme endpoint to a singular method, adds interpretation of static themes, and allow parent theme recursion. I am not 100% sure this will order styles correctly.
This updates the theme picker to include brews tagged as themes owned by the user. Some supporting functions were updated. User themes are loaded on /edit and added to the request.
|
Test theme: https://homebrewery.naturalcrit.com/share/qr7i-TXHV8yf |
Let me get this straight. You think it would be better to reassign the theme of a Brew to an arbitrary brew instead of removing theming and informing the user? As a user I would find that infuriating. |
No styling is a fallback value. It is the penultimate fallback. |
Not reassign their selection to an arbitrary theme. Just dont show a broken webpage if the one they pick doesn't exist. The app should have some robustness against errors like this. Same reason we put the brewRenderer in an iFrame. If a user messes up their HTML, allowing the whole webpage to crash would be bad design.
If No Styling is the penultimate, then I'm curious what you would consider the ultimate fallback after that? In any case, the Blank theme was kind of designed to be that minimum styling that is essentially "no style" while still appearing like a functioning page. I think it makes sense to display Blank if the currently-selected option doesn't exist. No, I dont mean change the selection out from under the user. But don't just show a broken page either. That would drive people crazy, and give the impression our site has just broken. |
Unless some of your rearrangement has broken existing behavior ( hadn't last I checked ) it does not break the site. I'm unsure where this assertion comes from.
There is none. No styling drops you back at the default behavior for your browser
Again with this broken site thing. |
|
The site isn't functionally broken, but visually broken, which is just as bad if a user can't tell the difference. With a full page of text, my first impression is that something crashed in the background, which is not the message we want to give. To me this is a bad user experience that we should avoid.
I think we are in agreement that it should fall back to looking "unstyled". But what "unstyled" looks like is the question under debate. The "browser default" as you call the above is not actually the browser default. We have CSS on the site that basically strips all of the browser defaults out, unsetting every property, to make it easier to build custom stuff. So this is even more unstyled than what people are used to. Browser defaults would at least have white background with black text, some line spacing, functioning headers, bold and italics, etc. The Blank theme is closer to Chrome defaults, which I do think conveys "unstyled", whereas the screenshot above conveys "broken" because it is missing most of the browser defaults people expect.
|
Now that we have a dedicated /theme/ route for the recursive theming, the CSS endpoint can be simpler for only getting the `style` of a single brew. naturalcrit#3075 already has this simpler version, but no testing, so I have copied this into a comment there for implementation when it is ready.
This PR adds tests which means we are now covering a larger % of the codebase. Raise the coverage thresholds to match.
I have added a proper popup if a theme, or any theme in the inheritance chain, fails to load. In this case, the brewRenderer falls back to displaying the unstyled Blank theme (but does not change the user's selection in the menu).
|
This lines up with my expectations and desires though it looks like we have to take a slightly more convoluted route to get there than I had in my head. |
|
After deployment of v3.14 bugs were reported, specially heroku error pages, due to timeout errors on load of undefined themes. Patch was rolled back. After fixing, was deployed again now, crashes seem to be avoided, but snippet bar seems to vanish for |
This adds the UI elements to the metadata selector for users to select brews tagged as
themeorThemeas a brew's theme.It also swaps in use of the CSS endpoints ( with small changes ) from #3303
Added Endpoints:
/theme/:renderer/:id/ - This endpoint provides a JSON object consisting of snippets (
.snippets) and styles (.styles) from the referenced combination of renderer and id. Both members return an array of contents from the referenced theme and its entire parental tree from oldest to newest.Testing: