Skip to content

Fix #1670 - Add option to manually assign an URL to a container #2710

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Fix #1670 - Add option to manually assign an URL to a container #2710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 43 additions & 0 deletions src/css/popup.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,13 @@
--input-border-color: #8f8f9d;
--input-border-focus-color: #0060df;
--input-border-active-color: #054096;
--input-destructive-border-color: rgba(226, 40, 80, 1);
--text-color-primary: #15141a;
--menu-bg-active-color: #cfcfd8;
--menu-bg-hover-color: #e0e0e6;
--messagebar-destructive-bg-color: rgba(255, 232, 232, 1);
--messagebar-destructive-border-color: rgba(0, 0, 0, 0.08);
--messagebar-destructive-text-color: rgba(21, 20, 26, 1);
--panel-bg-color: #fff;
--panel-separator-color: rgba(240, 240, 244, 1);
--link-color: #0060df;
Expand Down Expand Up @@ -154,11 +158,15 @@
--input-border-color: #8f8f9d;
--input-border-active-color: #aaf2ff;
--input-border-focus-color: #0df;
--input-destructive-border-color: rgba(255, 132, 128, 1);
--link-color: #0df;
--link-hover-color: #80ebff;
--text-color-primary: #fbfbfe;
--menu-bg-active-color: #5b5b66;
--menu-bg-hover-color: #52525e;
--messagebar-destructive-bg-color: rgba(105, 15, 34, 1);
--messagebar-destructive-border-color: rgba(255, 255, 255, 0.08);
--messagebar-destructive-text-color: rgba(251, 251, 254, 1);
--panel-bg-color: #42414d;
--panel-separator-color: rgba(82, 82, 94, 1);

Expand Down Expand Up @@ -447,6 +455,7 @@ table {

.button {
background-color: var(--button-bg-color-secondary);
border: none;
color: var(--text-color-primary);
}

Expand Down Expand Up @@ -2441,3 +2450,37 @@ tr:hover > td > .reset-button {
.searchbar input {
inline-size: 100%;
}

/* Assignment panel */
#edit-sites-new-assignment {
display: flex;
flex-direction: column;
gap: 8px;
margin: 8px;
Copy link

@kurahaupo kurahaupo Mar 12, 2025

Choose a reason for hiding this comment

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

margin disallowed by property-disallowed-list rule

}

#edit-sites-add-assignment {
block-size: 32px;
border: none;
border-radius: 4px;
font-family: inherit;
font-weight: bold;
text-align: center;
}

#edit-sites-add-assignment-info {

Copy link

@kurahaupo kurahaupo Mar 12, 2025

Choose a reason for hiding this comment

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

empty CSS block disallowed by block-no-empty rule

}

#edit-sites-add-assignment-error {
background-color: var(--messagebar-destructive-bg-color);
border: 1px solid var(--messagebar-destructive-border-color);
border-radius: 4px;
color: var(--messagebar-destructive-text-color);
display: none;
padding: 8px;
Copy link

@kurahaupo kurahaupo Mar 12, 2025

Choose a reason for hiding this comment

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

padding disallowed by property-disallowed-list rule

}

#edit-sites-add-assignment:hover {
cursor: pointer;
}
42 changes: 40 additions & 2 deletions src/js/popup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ const Logic = {
notificationCards.forEach(notificationCard => {
notificationCard.textContent = text;
notificationCard.classList.add("is-shown");

setTimeout(() => {
notificationCard.classList.remove("is-shown");
}, 2000);
Expand Down Expand Up @@ -1454,6 +1454,35 @@ Logic.registerPanel(P_CONTAINER_ASSIGNMENTS, {
return Promise.resolve(null);
},

async addAssignment() {
const errorMessage = document.getElementById("edit-sites-add-assignment-error");
const assignmentInput = document.getElementById("edit-sites-add-assignment-info");

let url = document.getElementById("edit-sites-add-assignment-info").value;

if (!URL.canParse(url)) {
url = "http://" + url;
}

if (new URL(url).protocol == "https:" || new URL(url).protocol == "http:") {
url = new URL(url);
} else {
errorMessage.style.display = "block";
assignmentInput.style.border = "2px solid var(--input-destructive-border-color)";
}

const userContextId = Logic.currentUserContextId();
await Utils.setOrRemoveAssignment(
false, url.origin, userContextId, false
Copy link

@TriMoon TriMoon Jan 17, 2025

Choose a reason for hiding this comment

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

url.origin can be non-existant/undefined due to the check above...

Copy link
Collaborator Author

@dannycolin dannycolin Jan 30, 2025

Choose a reason for hiding this comment

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

What "check" are you referring to?

Copy link

@TriMoon TriMoon Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

   if (new URL(url).protocol == "https:" || new URL(url).protocol == "http:") {
      url = new URL(url);
   } else {

As you can see you change url here to be an URL(), but before this check url was a string.
Thus you can't access the .origin member if it isn't a URL() yet 😉

Good thing my eyes were trained before we had IDE's that do type checking automatically 😸

);

const assignments = await Logic.getAssignmentObjectByContainer(userContextId);
this.showAssignedContainers(assignments);

errorMessage.style.display = "none";
assignmentInput.style.border = "1px solid var(--input-border-color)";
Comment on lines +1474 to +1483
Copy link

@achernyakevich-sc achernyakevich-sc Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like this block should be inside if (new URL(url).protocol == "https:" || new URL(url).protocol == "http:") { because if condition fail assignment is not required.

Though probably more correct would be to do not make any assumptions about protocol, but expect from user that if he manually add URL then it would be fully qualified URL containing protocol. So in general code flow could looks like (it is not a real code but pseudo-code):

let url = document.getElementById("edit-sites-add-assignment-info").value;

try {
    let urlObj = new URL(url);

    if (urlObj.protocol == "https:" || urlObj.protocol == "http:") {
        const userContextId = Logic.currentUserContextId();
        await Utils.setOrRemoveAssignment(
            false, urlObj.origin, userContextId, false
        );

        const assignments = await Logic.getAssignmentObjectByContainer(userContextId);
        this.showAssignedContainers(assignments);

        errorMessage.style.display = "none";
        assignmentInput.style.border = "1px solid var(--input-border-color)";
    }
} catch {
    errorMessage.style.display = "block";
    assignmentInput.style.border = "2px solid var(--input-destructive-border-color)";
}

Copy link

@TriMoon TriMoon Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@achernyakevich-sc
Why not use the below instead of your if clause? 😉

if (/^http(s)?::/.test(url)) {

The used regExp can easily be expanded/modified...

Copy link

@achernyakevich-sc achernyakevich-sc Jan 17, 2025

Choose a reason for hiding this comment

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

Why not use the below instead of your if clause? 😉

At first it is "pseudo-code" as I mentioned in the comment. :) I just used code that already exists and regrouped it to show more correct execution flow.

But I think RegExp is not necessary here and now. MAC works for HTTP and HTTPS protocols only (AFAIK). And now code is "easily readable". We don't need to spend effort to care about that will never happen. Use KISS approach - Keep It Simple Stupid.

P.S.> One of my England customers said years ago - if I have a car that can drive to China it does not mean that I need to travel to Beijing.

Copy link

@TriMoon TriMoon Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It must be personal preference then i guess, but i bet my code runs faster with less extra resource usage... 😛
All little bits help in the long run of any software..

"easily readable"

It is easy readable as can be for anyone that knows JavaScript well enough...

Copy link
Collaborator Author

@dannycolin dannycolin Jan 18, 2025

Choose a reason for hiding this comment

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

@achernyakevich-sc At first, I used exactly what you're proposing with a try...catch. However, I wanted the user to have the ability to omit http(s) similar to the previous PR for this feature. In this case, I had to check the URL validity and force one if it's missing. I feel like it makes it more intuitive for users that don't know that valid URLs need a protocol.

@TriMoon It's more readable as @achernyakevich-sc said. Also, the performane is negligeable in this case.

Copy link

@TriMoon TriMoon Jan 22, 2025

Choose a reason for hiding this comment

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

@dannycolin Anything is negligible on PC's these days with their power, when you look at each one on it's own, think about the bigger total 😉

But anyhow did you notice #2710 (comment) 🤔

Copy link
Collaborator Author

@dannycolin dannycolin Jan 22, 2025

Choose a reason for hiding this comment

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

This very specific case is fine. I'll try my best to look at your other comment during the weekend and make sur to report back on GitHub ;).

Copy link

@achernyakevich-sc achernyakevich-sc Jan 22, 2025

Choose a reason for hiding this comment

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

However, I wanted the user to have the ability to omit http(s) similar to the previous PR for this feature. In this case, I had to check the URL validity and force one if it's missing.

@dannycolin It radically changes how we need to implement it! Below I will try to summarize how it would be simpler (to my mind) for understanding by users and simpler for implementation. :)

I have made some research of MAC behaviour and didn't check the code. So I will make some assumptions that proposal below is based on (to lazy to check the code base). The main one - MAC records not a whole URL of site, not protocol+host name, but just site origin. It means that though MAC works for HTTP and HTTPS, but it will work the same for both URLs - http://example.com/ and https://example.com/. Conclusion - UI should not only provide possibility to omit protocol part, but should force to enter site origin without protocol and path name. Only site origin!

Currently saving is triggered by button click. As soon as user have made click then it should go the following way:

  • take url string that leading http:// or https:// will be removed.
  • get urlObject based on cleaned url and added explicit https://.
  • as we explicitly added https:// we don't need checking for protocol.
  • get site origin from urlObject and expose it to UI in input as cleaned up.

So user will see on UI exactly that will be saved as configuration and used for triggering container switching.

In this case code could looks like:

let urlInput = document.getElementById("edit-sites-add-assignment-info")
let url = urlInput.value.replace(/^http(s)?:\/\//, "");

try {
    let urlObject = new URL("https://" + url);
    urlInput.value = urlObject.origin;

    const userContextId = Logic.currentUserContextId();
    await Utils.setOrRemoveAssignment(
        false, urlObject.origin, userContextId, false
    );

    const assignments = await Logic.getAssignmentObjectByContainer(userContextId);
    this.showAssignedContainers(assignments);

    errorMessage.style.display = "none";
    assignmentInput.style.border = "1px solid var(--input-border-color)";
} catch {
    errorMessage.style.display = "block";
    assignmentInput.style.border = "2px solid var(--input-destructive-border-color)";
}

To make it even more explicit you could set placeholder text for the input as site origin and it will be visible before user enter anything there. :)

Copy link

@achernyakevich-sc achernyakevich-sc Jan 22, 2025

Choose a reason for hiding this comment

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

Anything is negligible on PC's these days with their power, when you look at each one on it's own, think about the bigger total

@TriMoon It is not a problem if something takes more time almost uncatchable for user but it as well takes electricity. I think you will agree that:

  • any operation on PC cause electricity costs.
  • wasting of electricity increases carbon footprints and produces a damage to nature.

Problem is that small destructive things happen many-many times! Firefox and MAC is popular. :)

For us it costs almost nothing to make piece of code more electricity-effective. But resulting effect to nature could be very valuable.

Copy link
Collaborator Author

@dannycolin dannycolin Jan 30, 2025

Choose a reason for hiding this comment

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

@achernyakevich-sc I agree. It makes sense to drop the protocol detection since we don't store it anyway. I'll refactor the patch to match your proposal. Thanks!

},

showAssignedContainers(assignments) {
const closeContEl = document.querySelector("#close-container-assignment-panel");
Utils.addEnterHandler(closeContEl, () => {
Expand All @@ -1464,6 +1493,12 @@ Logic.registerPanel(P_CONTAINER_ASSIGNMENTS, {
const assignmentPanel = document.getElementById("edit-sites-assigned");
const assignmentKeys = Object.keys(assignments);
assignmentPanel.hidden = !(assignmentKeys.length > 0);

const addAssignmentButton = document.getElementById("edit-sites-add-assignment");
Utils.addEnterHandler(addAssignmentButton, async () => {
this.addAssignment();
});

if (assignments) {
const tableElement = document.querySelector("#edit-sites-assigned");
/* Remove previous assignment list,
Expand All @@ -1487,6 +1522,7 @@ Logic.registerPanel(P_CONTAINER_ASSIGNMENTS, {
<img title="${deleteSiteInfo}" class="trash-button delete-assignment" src="/img/container-delete.svg" />
</td>`;
trElement.getElementsByClassName("favicon")[0].appendChild(Utils.createFavIconElement(assumedUrl));

const deleteButton = trElement.querySelector(".trash-button");
Utils.addEnterHandler(deleteButton, async () => {
const userContextId = Logic.currentUserContextId();
Expand All @@ -1496,6 +1532,7 @@ Logic.registerPanel(P_CONTAINER_ASSIGNMENTS, {
delete assignments[siteKey];
this.showAssignedContainers(assignments);
});

const resetButton = trElement.querySelector(".reset-button");
Utils.addEnterHandler(resetButton, async () => {
const cookieStoreId = Logic.currentCookieStoreId();
Expand All @@ -1510,6 +1547,7 @@ Logic.registerPanel(P_CONTAINER_ASSIGNMENTS, {
Logic.notify({messageId: "cookiesCouldNotBeCleared", placeholders: [site.hostname]});
}
});

trElement.classList.add("menu-item", "hover-highlight", "keyboard-nav");
tableElement.appendChild(trElement);
});
Expand Down Expand Up @@ -2319,7 +2357,7 @@ Logic.registerPanel(P_CLEAR_CONTAINER_STORAGE, {
// This method is called when the panel is shown.
prepare() {
const identity = Logic.currentIdentity();

// Populating the panel: name, icon, and warning message
document.getElementById("container-clear-storage-title").textContent = identity.name;
return Promise.resolve(null);
Expand Down
7 changes: 7 additions & 0 deletions src/popup.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,13 @@ <h3 class="title" id="edit-assignments-title" data-i18n-message-id="default"></h
</tr>
</table>
</div>
<div id="edit-sites-new-assignment">
<input id="edit-sites-add-assignment-info" type="text">
<p id="edit-sites-add-assignment-error">You entered an invalid site URL.</p>
<button id="edit-sites-add-assignment" class="button controller">
Add site
</button>
</div>
</div>

<div class="hide panel delete-container-panel" id="delete-container-panel">
Expand Down
Loading