Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `default-to-rich-diff` feature #1932

Merged
merged 16 commits into from May 3, 2019

Conversation

Projects
None yet
3 participants
@idasbiste
Copy link
Contributor

commented Apr 14, 2019

I've used a similar approach as the one in #1924. With MutationObserver we can watch changes in the DOM for files that are loaded asynchronously and added to the page a posteriori.

This functionality can be extended to other files (markdown ones, for example): just add the intended extension to the array defined in default-to-rich-diff.tsx::7.

Example of a PR with SVG files: elementary/icons#792 (as the issue's OP said "pretty much any PR at icon repos, such as https://github.com/elementary/icons/pulls")

Closes #1599

@bfred-it
Copy link
Collaborator

left a comment

Thanks for the PR!

Show resolved Hide resolved source/features/default-to-rich-diff.tsx Outdated
Show resolved Hide resolved source/features/default-to-rich-diff.tsx Outdated
Show resolved Hide resolved source/features/default-to-rich-diff.tsx Outdated
Show resolved Hide resolved source/features/default-to-rich-diff.tsx Outdated
Show resolved Hide resolved source/features/default-to-rich-diff.tsx Outdated
@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Apr 17, 2019

Can you rebase from master (we enabled TS strict mode) and fix any issues?

Eduardo Fernandes added some commits Apr 14, 2019

Eduardo Fernandes Eduardo Fernandes
Default to rich diff each SVG file in a pull request
* This functionality can be extended to other files: just add the intended extension in default-to-rich-diff.tsx
Eduardo Fernandes Eduardo Fernandes
Address PR comments
* Use selector to find SVG files for accessing the rich-diff button
Eduardo Fernandes Eduardo Fernandes

@idasbiste idasbiste force-pushed the idasbiste:default-to-rich-diff branch from b708304 to 446b8d5 Apr 17, 2019

Eduardo Fernandes Eduardo Fernandes
@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2019

I simplified the loading mechanism (I'd still like this moved to its own file), however I noticed a pretty big problem:

  1. If GitHub's JS isn't loaded, the click will cause the form to be submitted to an empty page and navigate away
  2. We can avoid this problem with preventDefault, but the Rich Preview button will remain unclickable and therefore the user can't see the preview at all

I added a setTimeout but that slows us down and doesn't necessarily prevent the problem.

@idasbiste

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

@bfred-it
We might be able to revert the button disable attribute after clicking:

// hacky...
button.addEventListener('click', event => setTimeout(() => {
	(event.target as Element).closest('button')!.removeAttribute('disabled')
}, 1000));

Or use MutationObserver on these elements looking for attribute changes and trigger an attribute removal if the element gets disabled.

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2019

That restores the button and at least the user can click it, even though I'd rather have a solution that keeps trying to load it. From my tests it looks like no further requests are triggered if we keep clicking it and re-enabling it until it's .selected

This would let us drop the 1000ms timeout.

You can use something like this where we have button.click()

button.click();
const interval = setInterval(() => {
	if (button.classList.contains('selected')) {
		clearInterval(interval);
	} else {
		button.disabled = false;
		button.click();
	}
}, 300);
@bfred-it
Copy link
Collaborator

left a comment

This seems to work for me!

Left before merging:

  • Description in readme and file header
  • @sindresorhus is the feature name ok?

@bfred-it bfred-it requested a review from sindresorhus Apr 24, 2019

bfred-it added a commit that referenced this pull request Apr 24, 2019

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Apr 26, 2019

@sindresorhus is the feature name ok?

👍

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented May 2, 2019

@idasbiste Can you commit to maintain this feature going forward? For example, in case GitHub changes something that breaks it. We already have way too many features to maintain, so we need to find a way to make this sustainable.

If so, can you add a line to the CODEOWNERS file?

Eduardo Fernandes added some commits May 3, 2019

Eduardo Fernandes Eduardo Fernandes
Update readme and tsx header with feature info
* Update CODEOWNERS to commit to maintain this feature in the future
Show resolved Hide resolved .github/CODEOWNERS Outdated
Show resolved Hide resolved readme.md Outdated

bfred-it and others added some commits May 3, 2019

Update .github/CODEOWNERS
Co-Authored-By: idasbiste <idasbistefernandes@gmail.com>

@bfred-it bfred-it changed the title Default to rich diff each SVG file in a pull request Add `default-to-rich-diff` feature May 3, 2019

@bfred-it bfred-it merged commit 3150af6 into sindresorhus:master May 3, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented May 3, 2019

🌈

@idasbiste idasbiste deleted the idasbiste:default-to-rich-diff branch May 3, 2019

@bfred-it bfred-it referenced this pull request May 18, 2019

Closed

Refined GitHub updates #1137

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.