Avoid window.open and add more visible eye icon #1996
Conversation
Signed-off-by: Paul Mannarino <[email protected]>
Signed-off-by: Paul Mannarino <[email protected]>
Signed-off-by: Paul Mannarino <[email protected]>
Signed-off-by: Paul Mannarino <[email protected]>
Signed-off-by: Paul Mannarino <[email protected]>
|
Please file against |
Signed-off-by: Paul Mannarino <[email protected]>
…open Signed-off-by: Paul Mannarino <[email protected]>
Signed-off-by: Paul Mannarino <[email protected]>
Signed-off-by: Paul Mannarino <[email protected]>
Signed-off-by: Paul Mannarino <[email protected]>
Signed-off-by: Paul Mannarino <[email protected]>
| <h4 class="modal-title" id="apiTokenModalHeaderLabel">API Token</h4> | ||
| </div> | ||
| <div class="modal-body"> | ||
| <pre><iframe id="apiTokenIframe" name="apiToken_iframe" src="scripts/pi-hole/php/api_token.php"></iframe></pre> |
There was a problem hiding this comment.
The QR code doesn't look great on mobile. Because it's built using a <table> the height can only be responsive to a certain extent. I tried using the createImage() function from the QR code lib being used but no dice.
Not sure how important this is, since if you're accessing the API token on mobile you most likely will be copying the raw string rather than using another mobile device to take a photo of the qr code.
There was a problem hiding this comment.
In my opinion it's an edge case. Generating a qr code with a mobile to be scanned with another mobile.
There was a problem hiding this comment.
Agreed. This is how it looks on my screen:
There was a problem hiding this comment.
Looks good. Could you add support for the theme's font color as we did in #1934. (Otherwise it's black font on black background in the dark themes)
There was a problem hiding this comment.
I think it might be safer to always use white background and black text for the <pre> element that displays the QRcode (even in dark themes).
There was a problem hiding this comment.
Updated to use a white background no matter the theme. My QR scanner worked with each theme. Thanks for pointing this out and brainstorming on this!
There was a problem hiding this comment.
Just a small nit: could could you remove (or color white) the small leftover margin. Best seen in LCARS theme
There was a problem hiding this comment.
It's the <pre> background-color.
You just need to add this into pi-hole.css:
#apiTokenModal pre {
background: #FFF;
}There was a problem hiding this comment.
It's also in the other themes!
|
pi-hole/pi-hole#4461 adjusts the |
Signed-off-by: Paul Mannarino <[email protected]>
paul-man
force-pushed
the
feat/avoid-window-open
branch
from
a30fcab to
1c12e97
Compare
Signed-off-by: Paul Mannarino <[email protected]>
|
Sorry for the inconvenience. The PHPStan erros should be fixed now. Please rebase on latest |
DL6ER
changed the title
|
This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there: https://discourse.pi-hole.net/t/pi-hole-ftl-v5-12-web-v5-9-and-core-v5-7-released/51795/1 |
By submitting this pull request, I confirm the following:
{please fill any appropriate checkboxes, e.g: [X]}{Please ensure that your pull request is for the 'devel' branch!}git rebase)git commit --signoff)What does this PR aim to accomplish?:
Resolve #1353
How does this PR accomplish the above?:
Replace usage of
window.open(url)andwindow.open(url, '_self')withwindow.location.href = urlIssue #1353 also had discussion of replacing the current logic for hiding pie chart slices so that it could work better on mobile. Updating usage of
window.open()seems so small that I'm not sure if it warranted 2 separate PRs so I've included both changes.What documentation changes (if any) are needed to support this PR?:
Maybe update the pie chart screenshot in the read.me to this:
