Closed oslego closed 3 years ago
Thanks for the PR, this looks excellent to me on first sight :)
@AbdullahDiaa You did most of the work on the extension, right? Do you think you could review these changes?
Can I get someone's attention to please review this PR?
I know little about the extension, and I'm not a committer, or I would.
Can I get someone's attention to please review this PR?
I would, however the recent changes to the beta api have broken your PR :-) Is this still relevant or should this be closed @oslego ?
I would, however the recent changes to the beta api have broken your PR :-) Is this still relevant or should this be closed @oslego ?
It's been a while. I won't update the commit to account for the new API changes so feel free to drop it.
Fix #29
Here's a proposal to refactor the popup for increased readability and the removal of jQuery.
There are no automated tests to run so I did my best to manually test the extension. The look and feel should be exactly as before (UI changes were out of scope for this PR). A few bugs were fixed along the way, but the win should be a code that's easier to follow and reason about.
Someone with more familiarity with the extension should do a few checks to ensure I haven't missed anything important.
The patches in this PR do a few things:
constants.js
file shared between background page andservicedata.js
to avoid needless duplication ofRATING_TEXT
;popup.html
andpopup.js
to remove the use of jQuery for DOM mutations and rely on vanilla JS instead. The JS code is restructured and<template>
HTML elements are used to better separate the markup and rendering methods which make use and populate them with data (incidentally fixes #27 by removing the use ofservice.name
in ID attributes);service.slug
instead ofserviceUrl
for the tosdr.org URL hash);I could have used plain JS template literals (ex:
<div>${point.title}</div>
), and then justinnerHTML
them to their containers, but the use of external data that can potentially contain unsanitized HTML worried me. The existingescapeHTML()
doesn't look too sophisticated against malicious payloads that could abuse the tosdr API to then be executed on the user's computer via the extension. So I chose to use<template>
instead. If a more complex UI was required, I'd perhaps suggest the addition of something like hyperHTML to simplify working with human-readable templates. But, for now, I believe this is sufficient given the reduced scope of the extension.These are quite a few changes already. I stopped short of doing more refactoring to ease the burden of code review. Particularly, the markup is mostly the same (with the exception of removing some unused
class
andid
attributes) and the CSS is left largely untouched. Both of these require additional cleaning up, but that should happen in a follow-up PR.As follow-ups, here's what I propose:
<dl>
) instead of an unordered list (<ul>
) with arbitrary markup inside.boostrap.min.css
and use more focused CSS to achieve the same UI without the added baggage.Let me know what you think.