Skip to content
This repository has been archived by the owner on Nov 17, 2022. It is now read-only.

fix tooltip #25

Merged
merged 1 commit into from Nov 29, 2021
Merged

fix tooltip #25

merged 1 commit into from Nov 29, 2021

Conversation

il3ven
Copy link
Contributor

@il3ven il3ven commented Nov 24, 2021

Fixes #15

@d80ep08th
Copy link

Is deleting the entire condition that makes the tooltip appear on screens of size 1024px or less the best way?
Im asking for my own wisdom because I figured out that the tooltip starts appearing right after 1168px point , which is like 144px more.

@il3ven
Copy link
Contributor Author

il3ven commented Nov 25, 2021

Im asking for my own wisdom because I figured out that the tooltip starts appearing right after 1168px point , which is like 144px more.

For me the tooltip stops appearing as soon as the size increases to more than 1024px. It is not visible at 1025px.

Is deleting the entire condition that makes the tooltip appear on screens of size 1024px or less the best way?

I don't see any problem.

@d80ep08th
Copy link

I don't see any problem.
I dont find the css readable at all but @TimDaub must have had a good reason for embedding "@media (max-width: 1024px) " in the first place, unless it was supposed to be a good first issue lol

@TimDaub
Copy link
Member

TimDaub commented Nov 29, 2021

the provenance for why I ended up using that mediaquery is this PR: https://github.com/emareg/classlesscss/pull/9/files

If I remember correctly, then this was the first proposal where the media query was used: emareg/classlesscss#8 (comment)

And the reason provided was to:

Since the title tooltip is not enabled on mobile devices,

Now, I wonder if it even makes sense continuing to fix this. On the classless repo, I've asked if it may make sense to use an entirely different concept of showing additional information that would also work on mobile devices properly: emareg/classlesscss#11

@TimDaub
Copy link
Member

TimDaub commented Nov 29, 2021

I'll accept this fix for now, but for the long term, I think we should go towards the direction outlined here: #25

@TimDaub TimDaub merged commit 66fd59d into rugpullindex:master Nov 29, 2021
@il3ven il3ven deleted the issue-15 branch November 29, 2021 19:46
@il3ven
Copy link
Contributor Author

il3ven commented Nov 29, 2021

I agree that tooltip is not the best solution and needs to replaced with something else.


Since the title tooltip is not enabled on mobile devices,

If this was the reason then I guess the code should have been this.

- @media (max-width:1024px)
+ @media (min-width:1024px)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tooltips only seem to work on small screens
3 participants