Visual Diff UI v1 #494

ericholscher · 2025-01-14T20:25:50Z

This adds a v1 Visual Diff UI.
It does the following:

Adds a UI element to track Docdiff state, and exposes filetreediff as a dropdown
Updates the URL to track diff state
Updates the UI element to show state based on events (eg. hotkeys)
Shows the current file in the dropdown if it's open, otherwise a generic message

Fixes https://github.com/readthedocs/meta/issues/171

Demo

Screenshare.-.2025-01-14.12_27_53.PM.mp4

This adds a v1 Visual Diff UI. It does the following: * Adds a UI element to track Docdiff state, and exposes filetreediff as a dropdown * Updates the URL to track diff state * Updates the UI element to show state based on events (eg. hotkeys) * Shows the current file in the dropdown if it's open, otherwise a generic message

humitos

This is, without any doubt, a lot better than what we had 😄 . It's tiny, simple, and usable.

src/docdiff.js

src/filetreediff.js

humitos · 2025-01-15T09:07:32Z

src/index.js

+    // Same for FileTreeDiff.
    hotkeys.HotKeysAddon,
-    docdiff.DocDiffAddon,
+    filetreediff.FileTreeDiffAddon,

    linkpreviews.LinkPreviewsAddon,
-    filetreediff.FileTreeDiffAddon,
    customscript.CustomScriptAddon,
+    docdiff.DocDiffAddon,


I'm not sure to understand this change. It seems that hotkeys addon was already initialized before filetreediff. Is there any other reason why you needed to re-order them here?

I think it's actually docdiff that needs to run last, because it's triggering the event based on the URL param and filetreediff is now listening to that to keep state. It's messy keeping state in multiple places with events like this, and I'm not convinced it's a good long term solution vs. having the docdiff addon keep the "enabled/disabled" state that other addons can query.

addons/src/docdiff.js

Lines 105 to 111 in 47645b0

// Enable DocDiff if the URL parameter is present

if (hasQueryParam(DOCDIFF_URL_PARAM)) {

const event = new CustomEvent(

EVENT_READTHEDOCS_DOCDIFF_ADDED_REMOVED_SHOW,

);

document.dispatchEvent(event);

}

docdiff addon keep the "enabled/disabled" state that other addons can query.

This will have similar problems. If the state changes on docdiff, you need to communicate this somehow to the other addons to update their UI as well. I don't think you can have a "reactive property" based on other web-component property.

Probably, what we need to share state is to use a ContextRoot, similar to what we are implementing in #491. That way, every time that any addons update the state all the other addons will be notified and act in consequence.

humitos · 2025-01-15T09:11:19Z

src/filetreediff.css

+  background-color: #f8f9fa;
+  padding: 0.25rem 0.75rem;
+  box-shadow: 0 2px 4px rgba(0, 0, 0, 0.1);
+  z-index: 2000;
+  border-radius: 0 0 6px 6px;


@agjohnson what would be the pattern here to add --readthedocs-* CSS variables? what are the correct/required ones to add? All of those with *color* on its name? Should all the "spacing" values be calculated from --readthedocs-filetreediff-font-size?

Whatever we're doing in other addons right now is fine. There isn't any issue with adding CSS variables that we'll change later especially if we aren't documenting them.

humitos · 2025-01-15T09:15:36Z

src/filetreediff.css

-}
-
-:host(.toast) > div {
+.file-dropdown {


I understand we are not using CSS classes for this, but more strict selectors instead. Example: :host > div > div > label and similars. I'm not 100% about the benefits, but just mentioning what's the pattern we have been following for other addons -- cc @agjohnson

Yea, I didn't have any understanding of the current pattern, so just went with something that worked. If we have a doc explaining the approach we're taking, happy to follow it.

Looking at the current CSS files, some have :host selectors and some don't, so wasn't sure what was best.

The classes just aren't usually necessary for such a sparse element, so class name is overly verbose. We know the structure is :host > div and :host > div > div, and there aren't any competing elements like div.directory-dropdown that make it necessary to name the elements.

This should follow what the other elements do at least and use more specificity, especially scoping the selector to start at :host (:host > div > div). This rules out any possibility of a deeper nested element to match the selector.

src/filetreediff.js

Co-authored-by: Manuel Kaufmann <humitos@gmail.com>

ericholscher · 2025-01-22T23:50:11Z

@humitos if you're happy with the overall concept here, I think we 👍 it and ship it, and we can iterate in dev. I'd like to start using visual diff more on WTD, and this UI makes it not a blocker :)

Can always add another issue for variables and CSS rules.

humitos

I re-reviewed this and I don't seeing anything blocking us to deploy this work. It's better to what we currently have and it has to be enabled per-project -- which looks safe.

Once it's deployed, we can ping some users so they can start testing it and give us some feedback (e.g. CPython folks are waiting for this, actually).

Let's ship it and iterate over it with the feedback we receive and our own as well 👍🏼

src/docdiff.js

ericholscher · 2025-01-23T17:43:17Z

Excited to test this out more! Always happy to update the code once we have patterns formalized.

agjohnson

Just noting some general patterns to follow, otherwise this PR looks great. I'm glad to have the update to replace the notification UI.

agjohnson · 2025-01-23T18:59:19Z

src/filetreediff.css

-}
-
-:host(.toast) > div {
+.file-dropdown {


The classes just aren't usually necessary for such a sparse element, so class name is overly verbose. We know the structure is :host > div and :host > div > div, and there aren't any competing elements like div.directory-dropdown that make it necessary to name the elements.

This should follow what the other elements do at least and use more specificity, especially scoping the selector to start at :host (:host > div > div). This rules out any possibility of a deeper nested element to match the selector.

agjohnson · 2025-01-23T19:00:18Z

src/filetreediff.css

+  background-color: #f8f9fa;
+  padding: 0.25rem 0.75rem;
+  box-shadow: 0 2px 4px rgba(0, 0, 0, 0.1);
+  z-index: 2000;
+  border-radius: 0 0 6px 6px;


Whatever we're doing in other addons right now is fine. There isn't any issue with adding CSS variables that we'll change later especially if we aren't documenting them.

agjohnson · 2025-01-23T19:37:43Z

src/filetreediff.js

+            />
+            Show diff
+          </label>
+          <select id="file-select" @change=${this.handleFileChange}>


file-select seems unused, are we doing something with this?

If this needs to be targeted with an event, use Lit @[event] attributes instead. It doesn't look like this is the case though.

agjohnson · 2025-01-23T19:49:28Z

src/filetreediff.css

+  flex: 1;
+  padding: 0.25rem 0.5rem;
+  border: 1px solid #ddd;
+  border-radius: 4px;


Generally, all units should be em/rem to avoid breaking scaling of these elements with font size -- px won't scale up if users override the font-size larger.

ericholscher requested a review from agjohnson January 14, 2025 20:47

humitos reviewed Jan 15, 2025

View reviewed changes

ericholscher added 2 commits January 15, 2025 08:53

Make Files Changed on selectable.

47645b0

Explain comment better

d8cb563

humitos reviewed Jan 15, 2025

View reviewed changes

src/filetreediff.js Outdated Show resolved Hide resolved

Update src/filetreediff.js

0244ac0

Co-authored-by: Manuel Kaufmann <humitos@gmail.com>

ericholscher marked this pull request as ready for review January 23, 2025 16:28

ericholscher requested a review from a team as a code owner January 23, 2025 16:28

humitos approved these changes Jan 23, 2025

View reviewed changes

src/docdiff.js Show resolved Hide resolved

ericholscher mentioned this pull request Jan 23, 2025

Integrate Visual Diff URL history #506

Closed

ericholscher merged commit 4af0887 into main Jan 23, 2025
4 checks passed

ericholscher deleted the ftd-dropdown branch January 23, 2025 17:42

agjohnson reviewed Jan 23, 2025

View reviewed changes

humitos mentioned this pull request Jan 27, 2025

Fix PR preview links python/python-docs-theme#215

Merged

	// Enable DocDiff if the URL parameter is present
	if (hasQueryParam(DOCDIFF_URL_PARAM)) {
	const event = new CustomEvent(
	EVENT_READTHEDOCS_DOCDIFF_ADDED_REMOVED_SHOW,
	);
	document.dispatchEvent(event);
	}

Visual Diff UI v1 #494

Visual Diff UI v1 #494

Uh oh!

Conversation

ericholscher commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Demo

Uh oh!

humitos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

humitos Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericholscher Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ericholscher commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

humitos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ericholscher commented Jan 23, 2025

Uh oh!

agjohnson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ericholscher commented Jan 14, 2025 •

edited

Loading

humitos Jan 16, 2025 •

edited

Loading

ericholscher Jan 15, 2025 •

edited

Loading

ericholscher commented Jan 22, 2025 •

edited

Loading

humitos left a comment •

edited

Loading