Skip to content

Visual Diff UI v1 #494

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 23, 2025
Merged

Visual Diff UI v1 #494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
340ef0a
Visual Diff UI v1
ericholscher Jan 14, 2025
47645b0
Make Files Changed on selectable.
ericholscher Jan 15, 2025
d8cb563
Explain comment better
ericholscher Jan 15, 2025
0244ac0
Update src/filetreediff.js
ericholscher Jan 15, 2025
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
11 changes: 11 additions & 0 deletions src/docdiff.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,11 @@ export class DocDiffElement extends LitElement {
return null;
}

// Update URL to include the diff parameter
const url = new URL(window.location.href);
url.searchParams.set(DOCDIFF_URL_PARAM, "true");
window.history.replaceState({}, "", url);

this.enabled = true;
this.originalBody = document.querySelector(this.rootSelector);
return this.compare();
Expand All @@ -209,6 +214,11 @@ export class DocDiffElement extends LitElement {
return null;
}

// Remove diff parameter from URL
const url = new URL(window.location.href);
url.searchParams.delete(DOCDIFF_URL_PARAM);
window.history.replaceState({}, "", url);

this.enabled = false;
document.querySelector(this.rootSelector).replaceWith(this.originalBody);

Expand Down Expand Up @@ -238,6 +248,7 @@ export class DocDiffElement extends LitElement {
this._handleHideDocDiff,
);
}

disconnectedCallback() {
document.removeEventListener(
EVENT_READTHEDOCS_DOCDIFF_ADDED_REMOVED_SHOW,
Expand Down
101 changes: 27 additions & 74 deletions src/filetreediff.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,86 +1,39 @@
:host > div {
margin: 1rem 0rem;
padding-top: 1rem;
padding-bottom: 1rem;
overflow: auto;
border-radius: 0.5rem;
font-family: "Lato", "proxima-nova", "Helvetica Neue", "Arial", "sans-serif";
font-size: 1rem;
color: rgb(64, 64, 64);
background-color: rgb(234, 234, 234);
}

:host > div > div.content > ul {
margin: 0;
padding-left: 20px;
}

:host(.toast) > div {
.file-dropdown {
Copy link
Member

@humitos humitos Jan 15, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@ericholscher ericholscher Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@agjohnson agjohnson Jan 23, 2025

Choose a reason for hiding this comment

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

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.

position: fixed;
padding-top: calc(1rem * 0.75);
padding-bottom: calc(1rem * 0.75);
margin: 0.75rem 0rem;
top: 7rem;
right: 2rem;
z-index: 1750;
font-size: calc(1rem * 0.85);
width: 35rem;
max-width: calc(100vw - 4rem);
}

@media (max-width: 640px) {
:host(.toast) > div {
right: 0.5rem;
}
}

:host > div > svg.header.icon {
height: calc(1rem * 2);
padding: 0.5rem 1.5rem;
float: left;
}

:host(.toast) > div > svg.header.icon {
height: calc(1rem * 1.5);
}

:host > div a {
color: rgb(8, 140, 219);
text-decoration: none;
}

:host > div > .title {
padding: 0.25rem 1rem;
margin-bottom: 0.25rem;
line-height: 1rem;
font-weight: bold;
top: 0;
right: 1rem;
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;
Comment on lines +5 to +9
Copy link
Member

@humitos humitos Jan 15, 2025

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

@agjohnson agjohnson Jan 23, 2025

Choose a reason for hiding this comment

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

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.

font-size: 0.85rem;
}
:host > div > div.content {
line-height: 1rem;
font-size: calc(1rem * 0.85);
padding-left: 66px;
}

:host(.toast) > div > .title {
padding: 0rem 1rem;
.diff-controls {
display: flex;
gap: 0.75rem;
align-items: center;
}

:host > div > .title > .right {
float: right;
.diff-controls select {
flex: 1;
padding: 0.25rem 0.5rem;
border: 1px solid #ddd;
border-radius: 4px;
Copy link
Contributor

@agjohnson agjohnson Jan 23, 2025

Choose a reason for hiding this comment

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

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.

appearance: revert;
}

:host > div > .title > .right > svg {
display: inline-block;
height: 1rem;
vertical-align: middle;
.diff-checkbox {
display: flex;
align-items: center;
gap: 0.35rem;
font-size: 0.85rem;
color: #333;
white-space: nowrap;
cursor: pointer;
color: rgba(96, 96, 96);
font-weight: normal;
}

:host(.raised) > div {
box-shadow:
0 2px 4px 0 rgba(34, 36, 38, 0.12),
0 2px 10px 0 rgba(34, 36, 38, 0.15);
.diff-checkbox input {
margin: 0;
}
191 changes: 104 additions & 87 deletions src/filetreediff.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { library, icon } from "@fortawesome/fontawesome-svg-core";
import { faCircleXmark, faFile } from "@fortawesome/free-solid-svg-icons";
import { html, nothing, render, LitElement } from "lit";
import { repeat } from "lit/directives/repeat.js";
import { html, nothing, LitElement } from "lit";
import { default as objectPath } from "object-path";
import styleSheet from "./filetreediff.css";
import { DOCDIFF_URL_PARAM } from "./docdiff.js";
import {
EVENT_READTHEDOCS_DOCDIFF_ADDED_REMOVED_SHOW,
EVENT_READTHEDOCS_DOCDIFF_HIDE,
} from "./events";
import { hasQueryParam } from "./utils";

import { AddonBase } from "./utils";

Expand All @@ -13,103 +15,136 @@ export class FileTreeDiffElement extends LitElement {

static properties = {
config: { state: true },
dismissed: { state: true },
docDiffEnabled: { state: true },
};

static styles = styleSheet;

constructor() {
super();
this.config = null;
this.dismissed = false;
}

firstUpdated() {
// Add CSS classes to the element on ``firstUpdated`` because we need the
// HTML element to exist in the DOM before being able to add tag attributes.
this.className = this.className || "raised toast";
this.docDiffEnabled = false;
}

loadConfig(config) {
if (!FileTreeDiffAddon.isEnabled(config)) {
return;
}

this.config = config;
}

render() {
if (this.dismissed) {
return nothing;
handleFileChange(event) {
const fileUrl = event.target.value;
if (fileUrl) {
const url = new URL(fileUrl);
// Only add the diff parameter if diff is currently enabled
if (this.docDiffEnabled) {
url.searchParams.set(DOCDIFF_URL_PARAM, "true");
}
window.location.href = url.toString();
}
}

library.add(faFile);
const iconFile = icon(faFile, {
title: "This version is a pull request version",
classes: ["header", "icon"],
});

const generateDiffList = (diffArray, label) => {
return diffArray.length
? html`
<span>${label}</span>
<ul>
${repeat(
diffArray,
(f) => f.filename,
(f, index) =>
html`<li>
<a href=${f.urls.current}>${f.filename}</a>
(<a href="${f.urls.current}?${DOCDIFF_URL_PARAM}=true"
>diff</a
>)
</li>`,
)}
</ul>
`
: nothing;
};
handleToggleDiff(event) {
if (event.target.checked) {
document.dispatchEvent(
new CustomEvent(EVENT_READTHEDOCS_DOCDIFF_ADDED_REMOVED_SHOW),
);
} else {
document.dispatchEvent(new CustomEvent(EVENT_READTHEDOCS_DOCDIFF_HIDE));
}
}

getCurrentPageUrl() {
// Remove any query parameters to match against file URLs
const currentPath = window.location.pathname;
const currentOrigin = window.location.origin;
return `${currentOrigin}${currentPath}`;
}

render() {
const diffData = objectPath.get(this.config, "addons.filetreediff.diff");
if (!diffData) {
return nothing;
}
const diffAddedUrls = generateDiffList(diffData.added, "Added");
const diffDeletedUrls = generateDiffList(diffData.deleted, "Deleted");
const diffModifiedUrls = generateDiffList(diffData.modified, "Modified");

const currentUrl = this.getCurrentPageUrl();
const renderSection = (files, label) => {
if (!files.length) return nothing;
const emoji = label === "Added" ? "+ " : "± ";
return html`
<optgroup label="${label}">
${files.map(
(f) => html`
<option
value=${f.urls.current}
?selected=${f.urls.current === currentUrl}
>
${emoji}${f.filename}
</option>
`,
)}
</optgroup>
`;
};

const hasCurrentFile = [...diffData.added, ...diffData.modified].some(
(f) => f.urls.current === currentUrl,
);

return html`
<div>
${iconFile.node[0]}
<div class="title">
Files changed in this version ${this.renderCloseButton()}
</div>
<div class="content">
${diffAddedUrls} ${diffModifiedUrls} ${diffDeletedUrls}
<div class="file-dropdown">
<div class="diff-controls">
<label class="diff-checkbox">
<input
type="checkbox"
.checked=${this.docDiffEnabled}
@change=${this.handleToggleDiff}
/>
Show diff
</label>
<select id="file-select" @change=${this.handleFileChange}>
Copy link
Contributor

@agjohnson agjohnson Jan 23, 2025

Choose a reason for hiding this comment

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

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.

<option value="" ?selected=${!hasCurrentFile} disabled>
Files changed
</option>
${renderSection(diffData.added, "Added")}
${renderSection(diffData.modified, "Changed")}
</select>
</div>
</div>
`;
}

renderCloseButton() {
library.add(faCircleXmark);
const xmark = icon(faCircleXmark, {
title: "Close notification",
});
return html`
<a href="#" class="right" @click=${this.closeNotification}>
${xmark.node[0]}
</a>
`;
}
_handleDocDiffShow = (event) => {
this.docDiffEnabled = true;
};

closeNotification(e) {
// Avoid going back to the top of the page when closing the notification
e.preventDefault();
this.dismissed = true;
_handleDocDiffHide = (event) => {
this.docDiffEnabled = false;
};

connectedCallback() {
super.connectedCallback();
document.addEventListener(
EVENT_READTHEDOCS_DOCDIFF_ADDED_REMOVED_SHOW,
this._handleDocDiffShow,
);
document.addEventListener(
EVENT_READTHEDOCS_DOCDIFF_HIDE,
this._handleDocDiffHide,
);
}

// Avoid event propagation
return false;
disconnectedCallback() {
document.removeEventListener(
EVENT_READTHEDOCS_DOCDIFF_ADDED_REMOVED_SHOW,
this._handleDocDiffShow,
);
document.removeEventListener(
EVENT_READTHEDOCS_DOCDIFF_HIDE,
this._handleDocDiffHide,
);
super.disconnectedCallback();
}
}

Expand All @@ -131,11 +166,8 @@ export class FileTreeDiffAddon extends AddonBase {

constructor(config) {
super();

this.config = config;
this.showDiff();

// If there are no elements found, inject one
let elems = document.querySelectorAll("readthedocs-filetreediff");
if (!elems.length) {
elems = [new FileTreeDiffElement()];
Expand All @@ -147,21 +179,6 @@ export class FileTreeDiffAddon extends AddonBase {
elem.loadConfig(config);
}
}

showDiff() {
// const outdated = objectPath.get(this.config, "addons.filetreediff.oudated", false);
const diffData = objectPath.get(this.config, "addons.filetreediff.diff");

for (let f of diffData.added) {
console.debug(`File: ${f.filename}, URL: ${f.urls.current}`);
}
for (let f of diffData.modified) {
console.debug(`File: ${f.filename}, URL: ${f.urls.current}`);
}
for (let f of diffData.deleted) {
console.debug(`File: ${f.filename}, URL: ${f.urls.current}`);
}
}
}

customElements.define("readthedocs-filetreediff", FileTreeDiffElement);
Loading