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

Add API notification support to new dashboard #300

Merged
merged 13 commits into from
Mar 26, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ commands:
steps:
- checkout
- run: pip install djhtml
- run: djhtml --tabwidth 2 readthedocsext/
- run: djhtml --check --tabwidth 2 readthedocsext/
run-build:
description: "Ensure built assets are up to date"
steps:
Expand Down
1 change: 1 addition & 0 deletions docs/api/javascript.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ JavaScript API

js/application
js/plugins
js/webcomponents
js/views
4 changes: 2 additions & 2 deletions docs/api/js/plugins.rst
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ Display

.. autofunction:: ./application/plugins.semanticui

.. autofunction:: ./application/plugins.webcomponent

Data
~~~~

Expand All @@ -25,8 +27,6 @@ How to pass data between the templates and our JavaScript in a secure manner.
Modules
~~~~~~~

.. autofunction:: ./application/plugins.message

.. autofunction:: ./application/plugins.chart

jQuery
Expand Down
14 changes: 14 additions & 0 deletions docs/api/js/webcomponents.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
Web components
==============

Elements
--------

.. js:autoclass:: NotificationListElement
:members:

.. js:autoclass:: NotificationElement
:members:

Views
-----
114 changes: 62 additions & 52 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@
"knockout": "^3.5.1",
"lato-font": "^3.0.0",
"less": "^4.2.0",
"less-loader": "^11.1.3",
"less-loader": "^12.2.0",
"lit": "^3.1.2",
"matchdep": "~2.0.0",
"mini-css-extract-plugin": "^2.7.6",
"prettier": "^3.0.3",
Expand Down

Large diffs are not rendered by default.

Large diffs are not rendered by default.

90 changes: 69 additions & 21 deletions readthedocsext/theme/static/readthedocsext/theme/js/vendor.js

Large diffs are not rendered by default.

16 changes: 8 additions & 8 deletions readthedocsext/theme/templates/account/login.html
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@

<div
{# The password URL is listed as an additional tab if present #}
class="ui small stackable secondary item menu {% if project_password_url %}three{% else %}two{% endif %}"
data-bind="semanticui: {tabs: {history: true, autoTabActivation: false}}">
class="ui small stackable secondary item menu {% if project_password_url %}three{% else %}two{% endif %}"
data-bind="semanticui: {tabs: {history: true, autoTabActivation: false}}">

{# This is purposely minimal, as the verbiage for "Log in with connected account" is a mountful #}
<a class="item" data-tab="vcs">
Expand All @@ -40,18 +40,18 @@
<a class="item {% if allowed_providers %}disabled{% endif %}"
data-tab="email"
{% if allowed_providers %}
data-content="{{ text_email_disabled }}"
aria-label="{{ text_email_disabled }}"
data-content="{{ text_email_disabled }}"
aria-label="{{ text_email_disabled }}"
{% endif %}>
<i class="fad fa-envelope icon"></i>
{% block authentication_email_text %}{% trans "Log in with email" %}{% endblock %}
</a>

{% if project_password_url %}
<a class="item" href="{{ project_password_url }}">
<i class="fad fa-lock icon"></i>
{% trans "Use a password" %}
</a>
<a class="item" href="{{ project_password_url }}">
<i class="fad fa-lock icon"></i>
{% trans "Use a password" %}
</a>
{% endif %}
</div>

Expand Down
4 changes: 2 additions & 2 deletions readthedocsext/theme/templates/base.html
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@

{% block content-wrapper %}
<div class="ui very padded container">
{% block messages %}
{% block notifications %}
{% include "includes/utils/messages.html" %}
{% endblock messages %}
{% endblock notifications %}

{# TODO move admin-bar usage to content-header #}
{% block admin-bar %}{% endblock %}
Expand Down
51 changes: 33 additions & 18 deletions readthedocsext/theme/templates/builds/build_detail.html
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
{% endblock %}

{% block content %}
<div class="ui basic segment padded" data-bind="using: BuildDetailView({id: {{ build.pk }}})">
<div class="ui basic segment padded" data-bind="using: BuildDetailView({}, '{% url "build-detail" build.id %}', '{% url "projects-builds-notifications-list" build.project.slug build.id %}')">

{% block build_detail_metadata %}
<div class="ui top attached segment">
Expand Down Expand Up @@ -202,25 +202,40 @@ <h3>{% trans "Build Errors" %}</h3>

{% else %}

{% comment %}
Error list
{% endcomment %}
{% if build.notifications.exists %}
{% for notification in build.notifications.all %}
{% if notification.get_message.type == "error" %}
<div class="ui attached inverted red segment">
<i class="fa-solid fa-exclamation circular icon"></i>
<span>{{ notification.get_message.get_rendered_body|safe }}</span>
<span>{% trans "For more information on this error, see our documentation." %}</span>
{% 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">
Comment on lines +205 to +216
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

<readthedocs-notification
inverted=true
csrf-token="{{ csrf_token }}"
data-bind="webcomponent: {notification: $data}">
</readthedocs-notification>
</div>

{% comment %}
Support for old style build errors, without notifications. In these
builds, ``error`` is a string and there is no header, so we use a
generic header instead.
{% endcomment %}
<div class="ko hidden ui attached fitted inverted basic segment" data-bind="css: { hidden: !error() }">
<div class="ui inverted error notification message">
<div class="header">
<i class="fad fa-circle-exclamation icon"></i>
{% trans "There was an error with this build" %}
</div>
{% endif %}
{% endfor %}
{% else %}
<div class="ui attached inverted red segment" data-bind="visible: error" style="display: none;">
<span data-bind="text: error"></span>
<span>{% trans "For more information on this error, see our documentation." %}</span>
<p data-bind="text: error"></p>
</div>
</div>
{% endif %}
{% endblock build_detail_notifications %}

<div class="ui inverted bottom attached segment transition slide">

Expand Down
17 changes: 0 additions & 17 deletions readthedocsext/theme/templates/includes/utils/messages.html
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,6 @@
page content pane.
{% endcomment %}

{% comment %}
``user_notifications`` comes from a Context Processor.
We need to use a CustomUser to have access to "user.notifications"
See https://docs.djangoproject.com/en/4.2/ref/settings/#std-setting-AUTH_USER_MODEL
{% endcomment %}
{% if user_notifications %}
{% for notification in user_notifications %}
<div class="ui message">
{% comment %}
Add this Xmark here once we implement dismissing notifications.
<i class="fa-duotone fa-circle-xmark close icon"></i>
{% endcomment %}
{{ notification.get_message.get_rendered_body|safe }}
</div>
{% endfor %}
{% endif %}

{% if messages %}
{% for message in messages %}
{% comment %}
Expand Down
Loading