Add API notification support to new dashboard #300

agjohnson · 2024-03-11T17:18:48Z

I started with a KO view for notifications, and the amount of boilerplate was
fairly heavy, as observables are needed for each notification attribute.

This is a quick test of using Lit for digestible chunks of HTML. I will say it's
way nicer than using KO views.

I'm wondering if it's worth testing Lit in this application. We're gaining more
knowledge on web components, and this is one of the use cases that seems like a
good fit.

Refs Notification: support new notifications through API #259

Work left here is all pretty minor:

Support build.error in the notification list when it's presented by the API
Clean up of FUI customizations, maybe set this up as a separate variant or maybe just see how much we've used .ui.message first of all.
Clean up some of the KO view leftovers as I've made changes as I moved through this
Come back to deal with Notifications: work around layout shift issues #313
Fix build detail notification extraneous polling issues
Fix notification overpolling issues with web component
Support query by state__in filter

agjohnson · 2024-03-11T17:23:38Z

@humitos I am putting this example because I wanted to see what you thought here. I can push up the basic KO view here too if you'd like comparison on the two approaches.

There are some downsides to this approach I feel -- mainly that this moves HTML templating into JS, which is harder.

However, there are no more KO data bindings to be aware of, it's just straight JS string/HTML template interpolation.

I will say it felt way easier to author this as a web component. If it feels like an upgrade, I feel it's worth testing the pattern a bit more and taking on Lit as a dependency (even if at least just temporarily).

humitos

I'm pretty biased on this and it's hard to not be subjective here.

I never felt comfortable with KO and each time I touch that code it feels confusing to me. In my opinion, WebComponents and Lit have been a lot more clear (not perfect, tho) than any other thing I've tried in JavaScript.

I understand this code a lot more easily and any other KO code you wrote on this project. I feel it's pretty explicit now I have more background on Lit.

readthedocsext/theme/templates/base.html

src/js/modules/notifications.js

agjohnson · 2024-03-13T17:04:00Z

I'm pretty biased on this and it's hard to not be subjective here.

Hah, yeah. I would say this is objectively better overall 😄

The places that I feel like this pattern is not obvious yet are:

Where we have complex KO views, like the BuildDetailView. There is a lot of nested HTML in the KO view, I don't yet know how a web component pattern would fit in here. I think a web component could replace this, but I don't yet know of the pattern.
And where we are mixing KO views and web components (temporarily?). In this PR, this is in the build detail view where a big KO view and a web component are mixed. Some logic needs to happen at the KO view, and the web component does conflict with that. Or it doesn't share a connection to the KO view.

I am going to experiment really quick with the second issue there on the build detail page. But if I don't make quick progress here, the build detail page might show notifications through a KO view instead.

This PR doesn't need to be blocked by using Lit, but I feel it's a pretty safe/obvious/isolated/low stakes place to experiment with moving towards Lit.

agjohnson · 2024-03-13T23:06:14Z

A pattern to make a hybrid KO view and Lit web component is actually rather straight forward:

<rtd-notification-list
  url="{% url "projects-builds-notifications-list" project.slug build.pk %}"
  csrf-token="{{ csrf_token }}"
  data-bind="webcomponent: {finished: !isPolling()}">
</rtd-notification-list>

This is a (custom) KO webcomponent data binding that applies the inverse of the observable isPolling to the web component property finished.

So, I think I proceed and the first use case needs some more thought, but I am not prioritizing replacing big KO views like that yet.

humitos · 2024-03-14T09:31:14Z

Wow 😲! It looks pretty straightforward. Is this a pattern that you designed or it's a standard way to set webcomponent attributes?

agjohnson · 2024-03-14T18:37:41Z

This is not standard, but it was also really similar to the other KO plugins in this repo. I reused the semanticui data binding plugin basically, just stripped down. I did see an old NPM package, and didn't set it up correctly while testing I eventually found, but by that time had a working plugin anyways.

I'll learn more while I play with it today, but the nice thing is that we can pass in whole data objects -- as opposed to passing in individual strings/booleans as you would normally do with web components. This feels like it will be really easy to integrate the two and slowly move towards Lit.

agjohnson · 2024-03-20T20:55:12Z

I am on to cleaning things up, but I am rather happy with the end result so far. Here is what the notification messages look like at the top of the page currently:

(Note this is just hacked HTML, if this were 5 separate messages the icons/text would be different)

The order of notifications here are: error, warning, info, note, tip. Note and tip notifications don't use the inverted styling as a way to appear less visually important - for optional tasks, where error/warning/info are for mandatory tasks/information.

On the build detail view, where the background in the area for errors is already inverted, note and tip have inverted styles:

I'm not yet settled if this should also replace the default .ui.message styles (which I don't care for). I might start this off as a separate .ui.message variant so that we don't override the places we have already used .ui.message to add supporting prose to forms/etc.

The style changes here are:

Message is single line to make for a more compact display
Header is inline, but bold to stand out
Link color matches the text color while inverted, instead of being link teal

Work left here is all pretty minor

humitos · 2024-03-21T09:34:24Z

On the build detail view, where the background in the area for errors is already inverted, note and tip have inverted styles:

I would use always the same rule no matter what page we are. Otherwise the thought behind the pattern becomes unnecessary complex and harder to understand for users.

The style changes here are:

Why not using the same style everywhere for simplification?

The style that have the header separated from the body looks pretty good --like the one you shared before in https://usercontent.irccloud-cdn.com/file/WLO4Qb75/image.png

Also, it seems these matches other "in page" notifications we are showing to users on settings pages and similar. IMHO, it makes sense to re-use these style as much as we can instead of creating a specific style for each of these notifications. From a user perspective will be easier to parse and for us to maintain the code.

agjohnson · 2024-03-21T16:57:26Z

Thanks. There is a lot to cover here on why these changes are needed and I don't know if it help to go through all of this process again. Overall, I don't want to get too stuck on the visual styles here though, I feel it's better to tune as we go, and the main reasoning is allow us to explore notifications more.

Otherwise the thought behind the pattern becomes unnecessary complex and harder to understand for users.

Users aren't going to know what tip or note blocks mean by the colors used either way, so this alone is not going to confuse users.

Why not using the same style everywhere for simplification?

Tip and note use a light background to help communicate to the user that they don't require attention. This is opposed to error, warning, and info, which stand out and pull the users attention away from everything else on the dashboard. The user should feel they need to do something with these immediately.

We are not using tip or note level messages yet either way however.

Tip level messages to me would be "You are using our beta" or "Have you tried Addons?". These styles allow for us to experiment with adding more frequent tip/note notifications without visually overwhelming the dashboard and user.

Also, it seems these matches other "in page" notifications we are showing to users on settings pages and similar. IMHO, it makes sense to re-use these style as much as we can instead of creating a specific style for each of these notifications.

I noted this above, this is already the end goal. I am isolating the work here to just notifications at the moment. Eventually the default .ui.message styles will be replaced with the light colored message here though. The reasons for this are:

Default .ui.message styles feel out of place and have bugs
- Styles don't match visually with other components like .ui.toast, .ui.segment, etc
- They use colors not used anywhere else in the theme
- Contrast is too low for accessibility standards on most variations
- Link colors do not match the message body color

The style that have the header separated from the body looks pretty good --like the one you shared before in https://usercontent.irccloud-cdn.com/file/WLO4Qb75/image.png

In addition to the reasons above regarding .ui.message, the reason the default styles need fixes here are:

The rounded corners on messages. There is no middle attached variant, so there are always rounded corners, which is sloppy for an inline component:

The header line separated takes up a lot of room and leaves a lot of dead space in the message. Space is already at a premium in the build detail view and at the top of the page where the more space we use the more likely we'll drastically shift the layout after partially loading the view.
The pastel colors on default messages look the most out of place here.

Anyways, this is all just background. Most important here is that the style changes here are quite minimal. I feel it's best to move forward and tune these as we actually start using things like tip/note messages.

…ntation This shows top level and build detail notifications with only some small bugs remaining.

- Adds build error handling in templates - Fixes polling bugs with build and notification API

humitos · 2024-03-25T11:27:17Z

I don't want to get too stuck on the visual styles here though, I feel it's better to tune as we go, and the main reasoning is allow us to explore notifications more

I don't want to block this work on this at all. I was just providing some feedback on what I saw without the intention of blocking it since the important piece here is to discuss about is the pattern itself, not the styling.

That said, thanks for all the explanation about the style and the reasons behind them 👍🏼

agjohnson · 2024-03-25T23:30:24Z

These tip/note notifications and styles will feel a bit new and out of place until we are using them more, and certainly something we can tweak as we go.

I've put more background on the continuation of this style work for other instances of .ui.message here:

Styles: continue altering base message styles #316

This would be something to chip away at later, it's not a strong priority.

agjohnson · 2024-03-25T23:37:52Z

Here's a wrap up of all of the various instances of notifications, dismissable and permanent.

Screencast.from.2024-03-25.16-34-11.webm

humitos

Looks good to me. It took me a while to review, but I think we are going in the right direction here. I'm sure we will come back later and make some improvements on this code, but it's ready for a v1 👍🏼 -- I'm excited to start testing this in production.

humitos · 2024-03-26T09:51:25Z

src/js/application/registry.js

@@ -27,12 +27,12 @@ export class Registry {
      console.error("View view_name is unspecified", view);
      return;
    }
-    this.views[view.view_name] = (params) => {
+    this.views[view.view_name] = (...params) => {


I don't know what ... means here. Do you have a link I can read about?

This is the spread operator: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax

It's synonymous with Python's *args basically. The change above changes this from a function that takes one argument to a function that takes any number of arguments.

src/js/modules/notifications.js

humitos · 2024-03-26T09:59:12Z

src/js/modules/notifications.js

+    const params = new URLSearchParams({
+      state__in: this.state,
+    });
+    this.request = fetch(`${this.url}?${params}`)


this.request is used just a flag to not keep polling if we already have the results, or it means something different?

Yup, exactly, this just prevents multiple polling.

Lit actually has a whole implementation for async tasks that seems worthwhile, but I didn't want to complicate things too much just yet:

https://lit.dev/docs/data/task/

I could see us using this later.

Maybe is it worth to add a TODO comment in the code with a link to this documentation page so it's easier to come back to this.

I opened up an issue:

Web component: handle async operations with built in Lit Task #320

I think we could explore this API usage here or even on Addons. It seems like a good improvement over a hand rolled solution, so maybe there isn't much blocking us. I mostly avoided it to avoid stacking complexities on this first pass. If we're sold, maybe I just bring this dependency in next time I do something with Lit (maybe one of the missing popupcards?).

Thanks for opening the issue.

I think we could explore this API usage here or even on Addons

I'm not sure how this applies to the addons since the only call we do to fetch the JSON object from the API is done at early load stage before even initializing the Lit elements and it's done only once and shared across all the Lit elements.

Can you expand about this here or open an issue on the addons explaining how we can use this?

We also do requests for the search API and docdiff, this could applicable there. It's a place we could decide to experiment with this, but I don't necessarily feel we need to change any patterns with Addons either.

Good point! Sounds good to me 👍🏼

src/js/application/elements.js

humitos · 2024-03-26T10:06:59Z

readthedocsext/theme/templates/builds/build_detail.html

+      {% block build_detail_notifications %}
+        {% comment %}
+          Build notification and error list
+
+          This uses ``readthedocs-notification`` directly, so that the build
+          detail view can handle all of the API requests in one place.
+
+          We pass the entire API notification response object into the web
+          component, which handles the data the same way as the notification
+          list element.
+        {% endcomment %}
+        <div class="ko hidden ui attached fitted inverted basic segment" data-bind="css: { hidden: !has_notifications() }, foreach: notifications">


It's not clear to me why we don't use readthedocs-notifications-list here. Can you expand on that? Do we plan to use it in the future?

I saw the polling is done via KO and each of the notification is rendered using readthedocs-notification with the data coming from KO --but I'm not sure to understand what's the thought behind that.

You are correct, given the current structure I could have used list here to do the API polling.

The reason I did not is because it seemed like we might be moving towards returning notifications in the build API v3 response -- so everything would happen with one API call. In this case, the readthedocs-notification-list element doesn't do anything.

If we decide to not do that, the polling can be pushed on to the list element, and the KO view can call into this element method instead of performing its own API call for notifications.

The reason I did not is because it seemed like we might be moving towards returning notifications in the build API v3 response -- so everything would happen with one API call

Do we have an issue for this? I suppose that ?expand=notifications on the request should be enough here.

Loosely, though there are a few individual issues combined here:

API: BuildCommands and missing fields to build v3 API readthedocs.org#11179

humitos · 2024-03-26T10:14:05Z

I'm merging changes from main and rebuilding the assets. I hope I'm doing this correctly 🙃

…notifications

agjohnson · 2024-03-26T17:30:50Z

Things look good, including built assets. Thanks for taking a look through this!

agjohnson force-pushed the agj/api-notifications branch from 5d3d6fc to daadd68 Compare March 13, 2024 03:48

humitos reviewed Mar 13, 2024

View reviewed changes

agjohnson added 6 commits March 21, 2024 16:19

Proof of concept Lit notification element

d67c76f

More experimenting with API notifications in other spots in the UI

4dcbb4b

WIP, includes web component for notification

8f77af5

Add notification message styles and overrides, mostly working impleme…

703aaf6

…ntation This shows top level and build detail notifications with only some small bugs remaining.

Clean up rough edges on build view, JS, and templates

d83651d

- Adds build error handling in templates - Fixes polling bugs with build and notification API

Relint files

58844f2

agjohnson force-pushed the agj/api-notifications branch from eabb09d to 58844f2 Compare March 21, 2024 23:22

Add missing file

08bea40

agjohnson force-pushed the agj/api-notifications branch from 4c9888e to 08bea40 Compare March 21, 2024 23:42

agjohnson added 4 commits March 21, 2024 17:29

Fix notification over polling

3f36762

Change API notification query to state__in

5cedcfc

Fix up docs

b08f08f

Drop extra messages block, it is unused.

dc0cac0

agjohnson marked this pull request as ready for review March 25, 2024 23:30

agjohnson requested a review from a team as a code owner March 25, 2024 23:30

agjohnson changed the title ~~Proof of concept Lit notification element~~ Add API notification support to new dashboard Mar 25, 2024

Add missing web components API doc file

6e34be9

humitos approved these changes Mar 26, 2024

View reviewed changes

Merge branch 'main' of github.com:readthedocs/ext-theme into agj/api-…

41a645d

…notifications

humitos merged commit 0f36e2e into main Mar 26, 2024
5 checks passed

humitos deleted the agj/api-notifications branch March 26, 2024 10:19

humitos mentioned this pull request Mar 26, 2024

Notification: support new notifications through API #259

Closed

2 tasks

agjohnson mentioned this pull request Mar 27, 2024

Web component: handle async operations with built in Lit Task #320

Open

1 task

humitos mentioned this pull request Mar 27, 2024

API: BuildCommands and missing fields to build v3 API readthedocs/readthedocs.org#11179

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API notification support to new dashboard #300

Add API notification support to new dashboard #300

agjohnson commented Mar 11, 2024 •

edited

Loading

agjohnson commented Mar 11, 2024

humitos left a comment

agjohnson commented Mar 13, 2024

agjohnson commented Mar 13, 2024

humitos commented Mar 14, 2024

agjohnson commented Mar 14, 2024

agjohnson commented Mar 20, 2024 •

edited

Loading

humitos commented Mar 21, 2024

agjohnson commented Mar 21, 2024 •

edited

Loading

humitos commented Mar 25, 2024

agjohnson commented Mar 25, 2024

agjohnson commented Mar 25, 2024

humitos left a comment

humitos Mar 26, 2024

agjohnson Mar 26, 2024

humitos Mar 26, 2024

agjohnson Mar 26, 2024 •

edited

Loading

humitos Mar 27, 2024

agjohnson Mar 27, 2024

humitos Mar 27, 2024

agjohnson Mar 27, 2024 •

edited

Loading

humitos Mar 27, 2024

humitos Mar 26, 2024

agjohnson Mar 26, 2024

humitos Mar 27, 2024

agjohnson Mar 27, 2024

humitos commented Mar 26, 2024

agjohnson commented Mar 26, 2024

Add API notification support to new dashboard #300

Add API notification support to new dashboard #300

Conversation

agjohnson commented Mar 11, 2024 • edited Loading

agjohnson commented Mar 11, 2024

humitos left a comment

Choose a reason for hiding this comment

agjohnson commented Mar 13, 2024

agjohnson commented Mar 13, 2024

humitos commented Mar 14, 2024

agjohnson commented Mar 14, 2024

agjohnson commented Mar 20, 2024 • edited Loading

humitos commented Mar 21, 2024

agjohnson commented Mar 21, 2024 • edited Loading

humitos commented Mar 25, 2024

agjohnson commented Mar 25, 2024

agjohnson commented Mar 25, 2024

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agjohnson Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agjohnson Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Mar 26, 2024

agjohnson commented Mar 26, 2024

agjohnson commented Mar 11, 2024 •

edited

Loading

agjohnson commented Mar 20, 2024 •

edited

Loading

agjohnson commented Mar 21, 2024 •

edited

Loading

agjohnson Mar 26, 2024 •

edited

Loading

agjohnson Mar 27, 2024 •

edited

Loading