Skip to content

Meta: Refactor custom tab highlighting#4918

Merged
cheap-glitch merged 11 commits intomainfrom
refactor-tab-highlighting
Nov 7, 2021
Merged

Meta: Refactor custom tab highlighting#4918
cheap-glitch merged 11 commits intomainfrom
refactor-tab-highlighting

Conversation

@cheap-glitch
Copy link
Contributor

@cheap-glitch cheap-glitch commented Oct 14, 2021

Follow-up of #4914 (comment) and #4914 (comment)

Test URLs

Navigating between tabs in any repo

Screenshot

capture-1634213450.mp4

Copy link
Contributor Author

@cheap-glitch cheap-glitch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR unifies some duplicated code between releases-tab and bugs-tab. Also the elementReady({stopOnDomReady:false}) is dropped in favor of a simple event listener. It seems to work pretty well.

@fregante you mentioned #4008 (comment), can you clarify? Does this mean adding an entry for the bugs tab in the more-dropdown-links menu?

@cheap-glitch cheap-glitch added the meta Related to Refined GitHub itself label Oct 14, 2021
@cheap-glitch cheap-glitch marked this pull request as ready for review October 14, 2021 14:24
@yakov116

This comment has been minimized.

@yakov116

This comment has been minimized.

@cheap-glitch

This comment has been minimized.

@yakov116

This comment has been minimized.

@cheap-glitch

This comment has been minimized.

@yakov116

This comment has been minimized.

@fregante
Copy link
Member

fregante commented Oct 16, 2021

This could be simplified, for example instead of using events, you can just call a function to re-update the nav bar.

Alternative we could use an additional init with deduplicate: false on both features that looks like:

function init () {
	if (shouldBugsTabHaveFocus) {
		// Remove focus from other tab
	}

	// Ensure that the bugs tab is consistent whether it needs it or not
	bugsTab.classList.toggle('current', shouldBugsTabHaveFocus);
	bugsTab.attributes.toggle('aria-current', shouldBugsTabHaveFocus); // Or whatever
}

shouldBugsTabHaveFocus would be calculated every time init is run and/or it could be memoized via mem to allow it to be called from the other init as well without doing calculations twice.

const shouldBugsTabHaveFocus = mem((url: string) => {
	///logic
})

@fregante fregante marked this pull request as draft October 16, 2021 15:41
@cheap-glitch
Copy link
Contributor Author

Alternative we could use an additional init with deduplicate: false on both features

That's already the case, the problem is ensuring that the bugsTab element has actually been added by the "main" init before trying to (un)highlight it.

A simpler solution could be to have a single un-deduplicated init that would:

  1. Check if the Bugs/Releases tab already exists, if not add it
  2. Then update its highlight

@fregante
Copy link
Member

the problem is ensuring that the bugsTab element has actually been added by the "main" init before trying to (un)highlight it.

append(bugsTab);
secondInit() // Call it manually, even if it's duplicate right now

A simpler solution could be to have a single un-deduplicated init that would:

That'd also work. As long as the pattern is preserved for releases-tab

@cheap-glitch cheap-glitch marked this pull request as ready for review October 19, 2021 09:08
Copy link
Contributor Author

@cheap-glitch cheap-glitch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went with the single-init solution.

@cheap-glitch cheap-glitch requested a review from fregante October 20, 2021 10:11
@fregante
Copy link
Member

fregante commented Oct 30, 2021

Up to you

Just merge after 21.11.1

@cheap-glitch
Copy link
Contributor Author

@refined-github/maintainers can someone else test this too, ideally in a browser other than Firefox? Thanks!

@yakov116

This comment has been minimized.

@yakov116

This comment has been minimized.

@fregante

This comment has been minimized.

@yakov116

This comment has been minimized.

@cheap-glitch cheap-glitch merged commit d903339 into main Nov 7, 2021
@cheap-glitch cheap-glitch deleted the refactor-tab-highlighting branch November 7, 2021 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

meta Related to Refined GitHub itself

Development

Successfully merging this pull request may close these issues.

3 participants