Skip to content

Improved: Show URL button#4238

Merged
Alkarex merged 15 commits intoFreshRSS:edgefrom
math-GH:improve-show-url-button
Mar 14, 2022
Merged

Improved: Show URL button#4238
Alkarex merged 15 commits intoFreshRSS:edgefrom
math-GH:improve-show-url-button

Conversation

@math-GH
Copy link
Copy Markdown
Contributor

@math-GH math-GH commented Feb 26, 2022

The show the website button is improved now:
grafik

Changes proposed in this pull request:

  • before: the button has a static URL. When user changed the URL in the text input the button opened the "old" URL. It needs a page reload to have the new URL available with the button
  • after: the button opens the shown URL from the text input
  • new: mouse title "show URL" to make it more understandable what is behind the button

How to test the feature manually:

  1. go to subscription management
  2. manage a feed
  3. change the URL of feed or website
  4. click the button
  5. the changed URL opens in the new tab

Pull request checklist:

  • clear commit messages
  • code manually tested

@math-GH math-GH added the UI 🎨 User Interfaces label Feb 26, 2022
@math-GH math-GH added this to the 1.20.0 milestone Feb 26, 2022
}
}

// set event listener on "show url" buttons
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems a bit complex, although presumably it would be possible to replace all of them with focus or click. (If not, I suspect you need focus in addition to the rest.)

It seems more intuitive to use the input event on the text/url input.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

mousedown and keydown would be enough in general. The mouseover is used to update the link preview.

(see browser corner)
grafik

I do not understand why it would be better to use the input event

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

focus is made for the purpose, mousedown and keydown is not. It's a more complex way of doing more or less the same thing hopefully identically and potentially slightly worse. mousedown and keydown should not be used in this way.

I do not understand why it would be better to use the input event

More correctly stated, the blur, change or input event.

You have three different behaviors:

  • mouseover to update preview, but this introduces a lot of insecurities, like what does tap & hold, copy link on a mobile browser do in terms of events, how buggy are they, could we get a flash of outdated URL
  • focus to update right before the actual click action
  • click to check if the URL is empty and focus the input field

But why bother with the hours of time spent testing assistive technologies and mobile browsers when you can just update the DOM on input?

Additionally if you don't do it on input it won't work as intended with the links panel or with link selectors, or bookmarklets such as these.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I changed to click event.


// set event listener on "show url" buttons
function init_url_observers() {
document.querySelectorAll('.open-url').forEach(function (btn) {
Copy link
Copy Markdown
Member

@Alkarex Alkarex Mar 5, 2022

Choose a reason for hiding this comment

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

Not super important because it is not our main page, but a loop to add event listeners is a bad sign. It is lighter to add a single event listener and use "event delegation" https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Building_blocks/Events#event_delegation

Several examples in e.g. this function:

FreshRSS/p/scripts/main.js

Lines 777 to 779 in da2adae

document.getElementById('aside_feed').onclick = function (ev) {
let a = ev.target.closest('.tree-folder > .tree-folder-title > a.dropdown-toggle');
if (a) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's also not uncommon to just set it all on document (with a similar event.target/event.target.matches/event.target.closest pattern). The overhead from a bit of if/else or switch or whatever is significantly less than adding an eventlistener on a gazillion elements,particularly in memory. But yeah, I doubt it matters here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it is the same like the show password buttons.

@Alkarex Alkarex merged commit bdf7e4d into FreshRSS:edge Mar 14, 2022
@math-GH math-GH deleted the improve-show-url-button branch March 14, 2022 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

UI 🎨 User Interfaces

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants