OrangeNote / RuneBook

📖 arcane Rune Pages manager for League of Legends 📜
MIT License
94 stars 65 forks source link

Runes tooltips #35

Closed fdarveau closed 6 years ago

fdarveau commented 6 years ago

I have added tooltips for runes, as mentionned in #32 .

The changes are based on your tooltips branch, except they are now HTML formatted and contain the full description. Here is one of the most complete example. Unefortunately, I haven't found a way to force them to the right, as you seemed to prefer based on your tooltip branch. I will push an update if I find a way to do so. In the meantime, they either appear above the rune (if enough space) or below (of not enough space above).

In order for it to work everywhere, I had to make a change to the championsgg plugin so the rune ID is an integer instead of a string since all other plugins were integers.

Note that I polluted the branch commit history by starting it from my other pull-request master (#33 ), then merging upstream (your repository) into my fork. I then reverted the commits from #33 so if you pull this one only, you won't pull #33 with it.

OrangeNote commented 6 years ago

Oh, that's great! I'll check the code later today. I'm really curious about why I didn't manage to make them work at first try.

Popup position is not a problem. The way you did you can still see all other runes in the row, that's good.

fdarveau commented 6 years ago

Basically, you went with standard (no JavaScript) tooltips of semantic-ui instead of using the JavaScript Pop-up version which allows a "data-html".

For the tooltip to appear, you then have to call a popup() function on the element you added the data-html tag.

https://semantic-ui.com/modules/popup.html

OrangeNote commented 6 years ago

Sorry, I didn't say it, but I know there's the JavaScript version of the popup and I tried it locally (no commits) but I didn't manage to make the popup appear in the page list section. That's why I decided to fallback to the CSS version.

I run your version, it works in current page section but not in page list section. So the problem is still there. Or does it work for you?

fdarveau commented 6 years ago

You are right, I just tested it.

I'm not sure what happened, but it did work last night. I might have done something wrong while trying to undo the mess I made when I realized I started off with the PR #33 version.

I will investigate and update you when it works again.

OrangeNote commented 6 years ago

That's what happens to me with your code: sometimes the popup doesn't show up at all; else if the rune is in the first row, the tooltip shows up at the top of the page, else it shows at the bottom of the page, causing overflow of the content.

schermata 2018-03-14 alle 14 51 01 schermata 2018-03-14 alle 14 51 14

I tried to set:

$('.tooltip').popup({
    inline: true
})

but with that option the tooltip starts from the correct element but has no offset along the row (I'm hovering over Perfect Timing, but the tooltip is pointing at the first of the row). schermata 2018-03-14 alle 15 28 37

fdarveau commented 6 years ago

Ok, with the League Client open, the tooltips work, probably because runes data is pulled from its API.

I'm not sure what causes the positioning issue on your end though. If it is a Mac vs Windows thing, I won't be able to fix it since I don't have a Mac =/.

If not enough space above : image

If enough space above : image

Same rune as first picture, but on 2nd row instead of 1st row, giving enough space above : image

Adding the "inline: true" doesn't change anything on my end : image.

It's a weird issue since using Electron should give the same result across platforms.

OrangeNote commented 6 years ago

That's a really weird behavior indeed... that's why I was completely lost. I'll give it another try and test personally on Windows too, to see all the differences between the two platforms and then decide what to do. Thanks!

fdarveau commented 6 years ago

Just saw the mess my messed up commit history caused after the PR merge.

Really sorry for the extra work there.

Lesson learned : create a branch from my fork before doing anything so the fork master branch can be branched from for another PR.

OrangeNote commented 6 years ago

Yeah, I forgot you did the reverts but no problem, I still had to work on it. Thanks for your help!