-
Notifications
You must be signed in to change notification settings - Fork 10
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
Customization patterns #51
Comments
My understanding here is that the main goal here is to customize the HTML structure of our own elements. Is that correct? I'm saying this because for style we have CSS classes/variables and for logic/data we have a discussion opened at #13 I'd personally avoid telling users to customize our elements by using their own JS class as suggested in Element template customization by JavaScript. I think most of our users won't have enough knowledge to do that. Maybe, theme authors, but that is a different conversation, if anything. The pattern that looks the most intuitive to me is Element template customization by inner HTML. I think it's easy to read by anyone that knows a little of HTML and can be customized with CSS in a simple way too. I'm not sure to understand the "security risk" here since the inner HTML will be written by the author themselves, so, why they would write risky code for themselves? 🤔 Going a little further here, since I like this pattern the most, I'd propose to use
I quickly tried the following code: <readthedocs-notification class="raised toast">
<div slot="content">
This content is changed by the user.
There are variable substitution as well.
See
<a href="${this.urls.build}">build's detail page</a>
or
<a href="${this.urls.external}">pull request #${this.config.versions.current.slug}</a>
for more information.
</div>
</readthedocs-notification> and it renders as So, it works! However, the immediate problem that we can see here is that it's not possible to use Lit expressions (e.g. In any case, IMO, this approach is pretty clean and we should try to make it work as we want. |
OK. It seems I'm not the only one wanting this: lit/lit-element#646. It's currently unsupported for different reason, being the main one XSS. However, there are people saying that this could be implemented by using a "template system" that works inside HTML, like using |
I think I found what I want after reading a lot on the internet 😅 I arrived at this issue lit/lit#867 and found these two different approaches:
I need to make a test now 😄 |
I would also avoid this pattern for customizing our elements, but would also say that the patterns above are for replacing our elements entirely. I still see the levels of customization as this:
The majority of users should end up in our basic use case. This level of customization is easier for us to predict and cover with backwards compatibility. Advanced users will require more direct manipulation of our data, and probably won't need to use our elements at all. I would put some version of templating as an intermediate customization pattern. It's not clear if we need it just yet though, and I could see us implementing this customization pattern later.
I do agree this pattern is easy and quite simple, which I like. What I don't like is that the template override pattern requires a specific version of our library, as the template context will change as we continue development. A project that customizes the template like this should really be using a pinned version of this library, so that their implementation doesn't deviate from the template context the library is providing. This is more of a problem with a template in HTML, as a user that doesn't touch their JS probably also doesn't touch their JS dependencies. Users and theme maintainers could still miss this with JS customization though.
This sounds like the issue I was pointing towards above with
This could be usable, I haven't spent much more time with this, but it does seem like slots would normally be used for appending child elements into our existing shadow DOM elements, not replacing all of our elements with the inner HTML.
Stampino looks like a great solution if we go implement the templating option. Overall, I lean towards providing a JS pattern to allow for advanced usage first, and gauging if another method for customization is required at all. |
The more I think about this the less I want people customizing our addons without clear control from ourselves about how to customize them. I would like, following the definitions we used previously in this issue, to re-define these types of customizations as:
This will reduce the maintenance burden and will give us full control on the addons, allowing us to "break something" and execute a data migration for Basic customizations if needed, while keeping Advanced customizations working since they are pinning the API JSON structure expected. My proposal is to implement the Basic and Advanced customization first and grow from there based on users' feedback. We are pretty close to get there already 👍🏼 |
Perhaps this is our intermediate customization use case? I'm not particularly sold on HTML attribute customization in either direction yet, so I'm willing to not rule this out just yet. For example, if a theme author wants to customize our addons for their theme users, the HTML attribute option could enable that without getting into JS customization. A theme author might also want to entirely replace our addons, and that is a good case for JS customization. I definitely agree we should not be pushing normal users towards this solution though. And in the end, there might not even be a strong use case for HTML attribute customization.
Big +1 from me here, I really like the idea of surfacing configuration in our UI.
Agreed. I think we might want to float the HTML attribute customization around and see what theme maintainers specifically feel about this. |
So, I am starting to see where an intermediate pattern fits more. We might be hurting adoption by not having an intermediate configuration option, which we might be noticing with not many maintainers working to integrate with out addons/flyout. Right now, we're only advertising two possibilities to theme maintainers:
In the middle of those patterns is the theme maintainer that doesn't want to maintain their own implementation but does want to dictate some of the flyout/addons configuration, and for the flyout and addons to visually fit in their theme. I would want to be here as a maintainer. But right now, we're not giving this maintainer any control. If a theme maintainer wanted to override an addon configuration attribute, they'd have to instruct their users how to edit the RTD addons configuration UI to match their theme's intended usage. This feels like a dead end for adoption for this type of maintainer. What I'd want as a maintainer is a way to override some of the attributes of the project's addons configuration so that users of my theme immediately had the configuration from my theme, in an easily reproducible way. I would like to experiment with two patterns:
|
Well, we made a big public announcement and contacted some of the theme authors just a few weeks ago. IMO, the lack of integration doesn't seems to be related with the "complexity" of the integration, but just because we haven't given them enough time yet. Most of the theme maintainers I contacted directly are happy having all the JSON data to integrate it as they want1. Also, it seems they are also taking the opportunity to re-design the integration (in a similar way as we want to do with our theme eventually: split version/language selectors), and that's probably another reason why it's going to take them more time to polish it.
As a theme maintainer I may be able to understand this position, but I would still have all the data to implement it as I want and base my integration on an API call that I know it's not gonna change over time --so, I would stick to that approach. However, the HTML layout or CSS would eventually change and may break my integration. However, as a Read the Docs maintainer, I'm not sure I want to provide such type of customization to theme maintainers. I want my users to have the full experience when they build their project on Read the Docs for the first time: all addons enabled by default. I don't want to loose the relationship with my users just because the theme maintainer decided to build a different integration on their theme. I want to give the users the ability to disable Read the Docs addons by themselves after seeing how they work and being conscious of that decision 2. I don't want to "remove the magic from our side and put the magic on the theme's side". I want to remove this magic and keep these configurations explicit and understandable for our users. IMO, users wanting only the theme flyout, should enable the theme flyout and disable the Read the Docs one. Users wanting the Read the Docs latest notification, should keep it enabled and disable the theme one. I clearly see how a magic mixture of them would end up with pretty confused users. One of the biggest benefits of the approach we built here is that we can deploy, in a simple way, a new release without re-building docs and without breaking any downstream integration (remember the HTML layout returned by Let's say that tomorrow we add a slider in the flyout to toggle DocDiff. If the theme is overriding that section we loose the opportunity to deploy this new feature to our users based on a theme maintainer's decision. I don't want to loose that relationship with my users. I want to have the power to communicate to my users the features we are building for them in a clear way. Following that vision, my position here is that I want to have full control over the addons we expose to our users and I want to give theme maintainers the ability to integrate with Read the Docs as they want on their theme using the API without interfering with our addons. I also want to give documentation authors full control on customization of our addons via the WebUI/dashboard. I want them to have the final decision here. Footnotes |
I haven't had time to come back to this conversation. I'm not disregarding your notes above, and there is a lot we overlap on there actually, but I did want to note a couple things while the conversation today is fresh:
To note what I was describing a bit more and touch on that last point while the conversation is fresh: Release patterns might be a helpful discussion, as what you're describing is the issue we had historically. The trouble has always been the conflict between injecting bleeding edge code and historical builds. Release structure might not be as complex as we're picturing:
And yes, this doesn't give the project the latest addons, but this is the same outcome as a theme taking full control and not implementing our features. If we give the theme a way to extend addons, it's more likely it will stay up to date. My preference is leaning towards a pattern that theme maintainers extend our classes I think. I do agree that giving the user ultimate control of addons makes sense, but perhaps there is a option between theme and user control. Maybe a config option? These are just some ideas though, not hard suggestions. |
Also worth noting that within the next year CSS https://caniuse.com/css-container-queries-style This allows for more logic inside CSS, basically /* On host DOM */
html { --readthedocs-tool-fixes: "docusaurus"; }
/* Shadow DOM */
header {
...
@container style(--readthedocs-tool-fixes: "docusaurus") {
background-color: red;
color: black;
}
} Chromium on the left, Firefox on the right: This pattern is currently usable for some positioning, as It feels like this pattern could eliminate much of the need for overriding our element with JS or HTML, and CSS customization is far easier to accomplish for both users and theme maintainers. |
From a discussion in #47
This is the beginning of a discussion of some customization patterns that we might provide theme maintainers and authors customizing the look and feel of their own documentation.
Some top level points:
Here are a few patterns:
Element template customization by JavaScript
Most obvious would be allowing customization of the class used for our element:
But this is heavy on the JS side too, of course.
Element template customization by HTML script
Something like
<script type="text/html">...</script>
is really approachable.There are some injection attack worries here to work through (see the Lit
unsafeHtml
tag/function for some background). This might influence this pattern.Element template customization by inner HTML
Not sure this is an option even, and it likely has the same trouble with unsafe HTML, but:
More granular Lit elements
I'm not even entirely sure on this one. It feels like JS API consumption would be easier.
But, for example, a Sphinx theme want something like this instead of a notification:
No element customization, push to JS API
The above example feels easier as the following pseudo library usage:
The text was updated successfully, but these errors were encountered: