Bubble on selection positioning

edouard-lopez commented 2 years ago

Thanks for adding this feature, it make it so easier to use!

Actual

I use other extension like signIt to translate selected text, and they are overlapping, below you can see La Bulle Elix is completely overlaping SignIt (we can see a part of the wing on the top left):

Expectation

The Simple Translate lets user choose where they want it display

Request

How feasible is it to be able to select the position?

hugolpz commented 2 years ago

The starting place is content_scripts/signit.js > selectionToHintIconCoords() (which you could hack on the x variable for a quick dirty fix). Also needs to add an option in popup/popup.js. And associated binding in background.js

Issues are :

to find the right ui
we start to have overload of advanced options while translations, wiktionary definition, and videos assets are not sufficient.
if we increasingly add such subtle option, we may need a proper option page.

Could be implemented either by :

an incremental .NumberInputWidget, similar to historyWidget, controling x OR y. Doc, Previous uses
a .DropdownWidget/.DropdownInputWidget select menu, similar to uiLanguageDropdown, controling x OR/AND y. Doc, Previous uses

Note: I'm pausing development on this add-on to refocus on other projects at the moment.

pushkar707 commented 8 months ago

Hi @hugolpz ,

I would like to work on this issue.

hugolpz commented 8 months ago

@pushkar707 hello, Please wait. We have to handle some paperwork before we are back into the GSoC. Also, one requirement of the GSoC is to do a microtask starting on March 18th. So better to wait then.

pushkar707 commented 8 months ago

@hugolpz okay, I understand that you are busy at the moment, so I won't comment on any issues for now, and keep making myself familiar to the codebase, then after 18th March maybe I will initiate the PRs

Kamalkishor0 commented 4 months ago

Hey @hugolpz , I would like to work on this issue.

hugolpz commented 4 months ago

Hello @Kamalkishor0 , the codebase is unstable since Kabir and myself work on it this summer. May I contact you again this automn ? we will plan for small tasks to distribute among volunteer developers.

saltykheera commented 3 months ago

@yug i am able to achieve this till now but on changing the top and bottom nothing is happening like no console.log is happening

my idea is when user select : top or bottom option

the commandshift (in Signit.js ) value can be conditionaly change to 0 or 32 this will move the bubble up or down

but plz check what am i doing wrong the value of the hint position dropdown is not working

hugolpz commented 3 months ago

Hello @saltykheera, Thank you for this. I'm on the road again heading to Poland's Wikimania 2024. I wont have much computer time. I can review your code tomorrow (08/01) evening or on 08/03. 👍🏼

hugolpz commented 3 months ago

Hello Salthy,

Myst of war

I have difficulties to see where were your changes are due to both prettyfier and behavior changes being in the same commit. Ignoring white space merely help since there are added linebreak and '->" changes all around.

The code is organized differently for your additions, on popup.js i can see:

all your code is together, which make mirroring and comparing with previous OO.ui.DropdownWidget (signLanguageDropdown anduiLanguageDropdown) code blocks harder #L294-L325
you do not reuse a change function #L477-L500 , you move to more direct code.
but i see a different pattern, instead of something such as await sendMessageUp("changeUiLanguage", newLanguage); you use sendMessageUp("storeParam", ["position", selectedPosition]);, which is not in line with other OO.ui.DropdownWidget* (?) and I don't know if supported.

Overall you are doing too much in one commit : 1. prettifying ; 2. adding a behavior ; 3. trying alternative coding way. (This with a junior dev teammate traveling abroad for conference, starts to make too much in one bit 😅 )

*: checking quickly on github

Missing labels

Maybe also, I don't see creation of the strings in ./i18n/en.json or any language. Since you use them, you have to add :

    "si-popup-settings-position":"Hint icon position:",
    "si-popup-settings-position-help":"Change default position for the hint icon.",

Into i18n/en.json#43 . The labels will only show up if your SignIt UI settings is set to English. We need to edit the other json so the label shows in other languages as well.

Missing values

Either add :

    "si-popup-settings-position-top":"Top",
    "si-popup-settings-position-bottom":"Bottom",

Or use in-JS international symbols:

▲
▼

As so :

      new OO.ui.MenuOptionWidget({
        data: "top",
        label: "▲",
      }),
      new OO.ui.MenuOptionWidget({
        data: "bottom",
        label: "▼",
      }),

saltykheera commented 3 months ago

@hugolpz currently i am ignoring i18 after i fix this issue i will add it later not the naming in used to to remind me of that . because i work in english and of i changed the language it is difficult for me to navigate

2> you are right abut the prettyfy thing and i will make sure i completed disable it before i make the next commit

3> i am getting your point of using missing label i will try again freshly am make the necessary commit and if required PR on thru morning positively

thanks 😊 for for time and help your commitment for work also pushes me

i try i wont be a ache next time with my code

hugolpz commented 3 months ago

No prob, I see the good will. 🥇 It's just a bit challenging to follow but it's good. 👍🏼

Simple trick if you ignore the i18n, then just comment it and hard code a label, like so :

new OO.ui.MenuOptionWidget({
  data: "bottom",
  label: "▼ Bottom"// await banana.i18n("si-popup-settings-position-bottom"),
}),

hugolpz commented 3 months ago

Thank to @saltykheera. Some posisiton labels to clean up a bit further.

lingua-libre / SignIt