Multiple Code Enhancements

lingua-libre / SignIt

🌻 Lingua Libre SignIt web-browser extension translates selected word in French Sign Language via an elegant pop up so you learn sign language while reading online.

https://addons.mozilla.org/en-US/firefox/addon/lingua-libre-signit/

MIT License

11 stars 13 forks source link

Multiple Code Enhancements #83

Closed kabir-afk closed 3 weeks ago

kabir-afk commented 3 weeks ago

Changes

5th June

e9c8dd - Replaced return with sendResponse function while sending records upon receiving the message signit.getfiles.
c83333 - As the commit message says, doing this enables the search functionality.
7216bf - Updated browser to chrome API for chrome to recognize the changes. FYI : Chrome namespace is recognized by Firefox as well.
c01b13 - Refined string split. Similar to what we did here in background-script.js.
2a8bb6 - SearchWidget placeholder relied on i18n to reflect the number of records ,sign language videos are present for. So updated i18n to reflect that.

7th June

As mentioned in #82 , changed the CSS to use Unicode references as they work in both popup and content scripts , don't violate any CSP and work across all browsers. They also adapt to UI library's CSS.

hugolpz commented 3 weeks ago

There are a lot of tiny bits so I will only review those which puzzle me a bit, with ideas when I have.

Q1: 7216bfa568707bda5f724432ebc73ee5c06fd69f : We also have a generic local browser. as a switch between web extension chrome. on chrome-based and web extension browser. on Firefox. See https://github.com/lingua-libre/SignIt/blob/master/popup/popup.js#L4 . If Firefox recognizes chrome., let's ditch out that switch and go for chrome. alone, no ?
Q2: 2a8bb61f17352ec3b8c5d89c147a2ec1e642ff2e Idea : make it more generic with array for when there are $1, $2, $3, $n.
Q3: 67a193bd1ea0ed037437ee72f1ff89db7c0dfa29 place the unicode character directly in HTML ?

kabir-afk commented 3 weeks ago

Here are my answers to the above questions :

Q1 : I agree that we can ditch out on the browser namespace altogether, but as per mdn , we should stick to it if aiming for cross browser compatibility. I'll update it later as per browser namespace, such that it works across both the browsers.
Q2: OK 👍
Q3: I initially thought the same ,but since buttons are constructed using the library and we don't have full freedom over the elements ,I went with updating the CSS.

hugolpz commented 3 weeks ago

@kabir-afk , ok for Q1, Q2, Q3. As for Q3 on CSS+oojs, please note that oojs will / should be ditched out somedays. Maybe in coming weeks if we have time.

kabir-afk commented 3 weeks ago

@hugolpz What will we be replacing it with ?

hugolpz commented 3 weeks ago

@kabir-afk Vanilla js / html / css.