Fix sticky file header tooltips by wsmd · Pull Request #1581 · refined-github/refined-github

wsmd · 2018-10-13T21:46:55Z

There's a minor issue causing tooltips of elements in the sticky file header (.file-header) to be cut-off. This happens because of the sticky position that Refined Github applies to the header.

A single file

Pull Request files

Solution

This PR attempts to resolve the issue by adding a workaround that reverses the direction of the tooltips. This only applies to:

A single file page
Pull Requests files page

https://github.com/sindresorhus/refined-github/blob/f8abd1e5d7c9d3b45bf3f49e58244e01b22938ca/source/content.css#L666-L668

https://github.com/sindresorhus/refined-github/blob/f8abd1e5d7c9d3b45bf3f49e58244e01b22938ca/source/content.css#L673-L675

After

Closes #1295 (also see: #1308, #689)

wsmd · 2018-10-13T21:48:25Z

 	}

+	if (pageDetect.isSingleFile() || pageDetect.isPRFiles()) {
+		enableFeature(fixStickyFileHeaderTooltips);


I'm not sure if this the right place to call this script, or even if the script should be added to /features, because this isn't necessarily a "feature", but rather a fix for another feature – the sticky file-header, that is. Any thoughts?

That's right, I think if this PR gets accepted, it will have to becomes its own feature: sticky-file-headers or even just sticky-headers that injects the CSS

https://github.com/sindresorhus/refined-github/blob/f8abd1e5d7c9d3b45bf3f49e58244e01b22938ca/source/content.css#L666-L668

and also fixes the issues it causes.

That, as previously mentioned, only if it's too much trouble to fix with CSS alone, which would be preferable.

That, as previously mentioned, only if it's too much trouble to fix with CSS alone, which would be preferable.

This is very interesting! I can't think of any way to get around this issue with CSS alone, and if there is a way to do it, I don't think it would be as straightforward as the JS approach of flipping the tooltip class names from "north" to "south".

What I don't like about this PR so far is the fact that the current implementation/script fix-sticky-file-header-tooltips isn't a feature!

I think if this PR gets accepted, it will have to become its own feature: sticky-file-headers or even just sticky-headers that injects the CSS

I very much like the idea of encapsulating everything about the sticky headers to its own feature! Thanks for suggesting it! 🙇

That said, a feature like that requires CSS. The challenge here is that: A) currently all CSS live in one place, and B) there is no easy way to tie CSS to the script of a specific feature.

Having something like that is not a very bad idea, IMO! I have given this some thoughts, and I have some ideas that I'd like to share in a separate issue outside this PR since this conversation could apply to any feature, not just this one.

wsmd · 2018-10-13T21:50:45Z

+const oppositeDirections = {
+	'tooltipped-n': 'tooltipped-s',
+	'tooltipped-ne': 'tooltipped-se',
+	'tooltipped-nw': 'tooltipped-sw'


See: https://styleguide.github.com/primer/components/tooltips/#tooltip-direction

wsmd · 2018-10-13T22:16:16Z

I also noticed that some related tooltips were intentionally removed (#689 (comment), #799, #1234) to get around this issue. Could we use this workaround/idea instead, and restore some of those tooltips?

Proof of concept:

https://github.com/sindresorhus/refined-github/blob/f8abd1e5d7c9d3b45bf3f49e58244e01b22938ca/source/content.css#L36-L39

  [aria-label='Edit comment']::after,
- [aria-label='Open this file in GitHub Desktop']::before,
- [aria-label='Open this file in GitHub Desktop']::after,
  [aria-label$='branch to make changes.']::before,

fregante · 2018-10-14T02:46:30Z

+
+export default function () {
+	for (const el of select.all('.file-header [class*=tooltipped-n]')) {
+		let direction;


Lo-fi one-liner to replace everything in this loop

el.className = el.className.replace('tooltipped-n', 'tooltipped-s');

I like how explicit the current implementation is, but I understand that it might be a bit too verbose. 👍

I'll replace it with your suggestion, and probably add some comments for future maintainers to explain what it's exactly doing and emphasizing that we're replacing all north directions, not just "n" to "s", especially that, at a glance, el.className.replace looks very close to el.classList.replace but it behaves very differently.

wsmd · 2018-10-14T04:05:34Z

-[aria-label='Open this file in GitHub Desktop']::before,
-[aria-label='Open this file in GitHub Desktop']::after,
-[aria-label$='branch to make changes.']::before,
-[aria-label$='branch to make changes.']::after,


Since I got a thumbs up from @bfred-it on #1581 (comment), I went ahead and reverted all existing overrides that were hiding the tooltips.

Tooltips should look like this now 🎉

The last two can still be hidden. The tool tips don’t add any more information and the icons are obvious.

The last two can still be hidden.

I agree 👌. While most of the tooltips listed there (now removed) were added because of this specific issue within file-header, some other tooltips, like the notification indicator for example, can be redundant.

All of these selectors should be restored, the icons are self-explanatory:

[aria-label^='View the whole file']::before, [aria-label^='View the whole file']::after, [aria-label^='Change this file']::before, [aria-label^='Change this file']::after, [aria-label='Delete this file']::before, [aria-label='Delete this file']::after, [aria-label='Edit this file']::before, [aria-label='Edit this file']::after,

fregante · 2018-10-15T03:35:31Z

A couple of things I'd rather see:

document.documentElement.classList.add('rgh-sticky-header') instead of selecting all elements manually (which wouldn't work on files that load later on big diffs)
use select.all('.file-header [class*=tooltipped-n], .table-list-header [class*=tooltipped-n]') in the main loop

wsmd · 2018-10-15T03:44:23Z

oooh absolutely! good catch on the first point!

I'll also rename the sticky-file-header to make-headers-sticky for consistency with make-discussion-sidebar-sticky. This feature is also for search headers, not just file headers. Let me know what you think.

fregante · 2018-10-15T03:46:46Z

Better yet... this whole feature can be left in the CSS by just overriding the position via CSS:

.file-header [class*=tooltipped-n]:before,
.table-list-header [class*=tooltipped-n]:before { 
    top: auto !important;
    bottom: -5px !important;
    transform: rotate(180deg)
}

.file-header [class*=tooltipped-n]:after,
.table-list-header [class*=tooltipped-n]:after { 
    top: 100% !important;
    bottom: auto  !important;
    margin-top: 5px  !important;
}

wsmd · 2018-10-15T03:53:08Z

Aside from making this whole a thing its own feature, which isn't a bad idea. I'm curious, why do you think that approach ☝️ is better than the following?

for (const el of select.all('.file-header [class*=tooltipped-n]')) {
  el.className = el.className.replace('tooltipped-n', 'tooltipped-s');
}

I feel like renaming tooltipped-n* to tooltipped-s* is likely to be future proof. If Github decides to change a tiny bit of their CSS, this extension has to be updated if we went with manually hard coding CSS properties. The style for that already exists.. it's tooltipped-s*, why should we replicate it?

wsmd · 2018-10-15T04:11:41Z

+		pageDetect.isPRSearch() ||
+		pageDetect.isSingleFile()
+	) {
+		enableFeature(makeHeadersSticky);


I'll move this to the top of ajaxedPagesHandler since the original styles were applied globally anyways. I don't think there's a need to limit this to certain pages.

fregante · 2018-10-15T04:18:41Z

If Github decides to change a tiny of their CSS

That's indeed a problem, but that also applies to JavaScript code and that usually requires more work to fix anyway.

The CSS-only solution is indeed "dirtier" (like my earlier suggestion to use className.replace) but perhaps we can agree that it's more efficient: it works across the board wherever those selectors apply, which means they also work on ajaxed-in files without further effort.

That said, the latest iteration of your PR seems rather lean so let's see what others say.

wsmd · 2018-10-15T04:26:39Z

Cool! I don't necessarily disagree with any of that. Like you said, the CSS solution does feel a bit dirtier (idk, all those !important feel a bit overwhelming haha).

I'm very happy with how this PR has turned out! Thanks a lot for the help! :)

I look forward to others feedback as well!

fregante

After this change it can be merged.

fregante · 2018-11-06T10:40:37Z

-[aria-label='Open this file in GitHub Desktop']::before,
-[aria-label='Open this file in GitHub Desktop']::after,
-[aria-label$='branch to make changes.']::before,
-[aria-label$='branch to make changes.']::after,


All of these selectors should be restored, the icons are self-explanatory:

[aria-label^='View the whole file']::before, [aria-label^='View the whole file']::after, [aria-label^='Change this file']::before, [aria-label^='Change this file']::after, [aria-label='Delete this file']::before, [aria-label='Delete this file']::after, [aria-label='Edit this file']::before, [aria-label='Edit this file']::after,

sindresorhus · 2018-11-06T15:44:00Z

Thanks for fixing, @wsmd :)

Fix sticky file header tooltips

e2e13e4

wsmd commented Oct 13, 2018

View reviewed changes

Add missing semicolon

3ce7797

fregante reviewed Oct 14, 2018

View reviewed changes

wsmd added 2 commits October 14, 2018 00:04

Simplify logic by using className instead of classList

8b18883

Revert tooltips style overrides

0519e24

wsmd commented Oct 14, 2018

View reviewed changes

Capitalize comments

7202341

wsmd mentioned this pull request Oct 14, 2018

Feature specific CSS: Coupling CSS to a feature script #1582

Closed

wsmd added 2 commits October 14, 2018 23:14

Add feature sticky-file-header

ce701a7

Enable feature sticky-file-header on PR search page

5c5824e

Additional refactor and rename feature

08bef8f

wsmd added 2 commits October 15, 2018 00:06

Update comments

3da76d2

Move makeHeadersSticky to onDomReady

7d9c6b2

wsmd commented Oct 15, 2018

View reviewed changes

Move feature to ajaxedPagesHandler to fix ajaxified pages

d1ca1e6

Remove unnecessary !important

75c9a1a

fregante mentioned this pull request Nov 6, 2018

Tooltip not visible on PR collapse file diff button #1609

Closed

fregante approved these changes Nov 6, 2018

View reviewed changes

wsmd and others added 2 commits November 6, 2018 09:15

Revert to hiding some self-explanatory icons

8364df4

Update make-headers-sticky.js

ea07052

sindresorhus merged commit a9ec680 into refined-github:master Nov 6, 2018

Uh oh!

Conversation

wsmd commented Oct 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

A single file

Pull Request files

Solution

After

Uh oh!

wsmd Oct 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fregante Oct 14, 2018

Choose a reason for hiding this comment

Uh oh!

wsmd Oct 14, 2018

Choose a reason for hiding this comment

Uh oh!

wsmd Oct 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wsmd commented Oct 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fregante Oct 14, 2018

Choose a reason for hiding this comment

Uh oh!

wsmd Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wsmd Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fregante Oct 14, 2018

Choose a reason for hiding this comment

Uh oh!

wsmd Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fregante Nov 6, 2018

Choose a reason for hiding this comment

Uh oh!

fregante commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wsmd commented Oct 15, 2018

Uh oh!

fregante commented Oct 15, 2018

Uh oh!

wsmd commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wsmd Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fregante commented Oct 15, 2018

Uh oh!

wsmd commented Oct 15, 2018

Uh oh!

fregante left a comment

Choose a reason for hiding this comment

Uh oh!

fregante Nov 6, 2018

Choose a reason for hiding this comment

Uh oh!

sindresorhus commented Nov 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

wsmd commented Oct 13, 2018 •

edited

Loading

wsmd Oct 13, 2018 •

edited

Loading

wsmd Oct 13, 2018 •

edited

Loading

wsmd commented Oct 13, 2018 •

edited

Loading

wsmd Oct 14, 2018 •

edited

Loading

wsmd Oct 14, 2018 •

edited

Loading

wsmd Oct 14, 2018 •

edited

Loading

fregante commented Oct 15, 2018 •

edited

Loading

wsmd commented Oct 15, 2018 •

edited

Loading

wsmd Oct 15, 2018 •

edited

Loading