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

Jetpack live branches: fix, simplify and refactor #11761

Merged
merged 1 commit into from
Mar 31, 2019

Conversation

simison
Copy link
Member

@simison simison commented Mar 29, 2019

I had a small evening inspiration moment so I went on and worked a bit on this script.

Use of GitHub's task-list-item, task-list-item-checkbox etc classes was sometimes causing Live branches to fail to work; it's a bit hard to replicate as it's probably a race condition depending on if Tampermonkey or GitHub's scripts load first.

While at it, I refactored and added more settings and plugins.

I hope the code is now also easier to grok and extend with these changes.

Before

before

After

after

Changes proposed in this Pull Request:

  • Add more settings
  • Add more plugins
  • Remove GitHub's own class names from HTML; this was causing a fatal error in the page each time checkboxes were toggled
  • Wrap checkbox texts in the label to make them clickable
  • Tell linter that jQuery is a global variable
  • Change how the link is structured
  • Change how HTML is structured
  • Move link under the settings from above
  • Remove language repetition so it's easier to quickly see where is what ("launch with", "plugin" etc)

Testing instructions:

Add script to Tampermonkey and refresh this page. :-)

Proposed changelog entry for your changes:

- Add more settings
- Add more plugins
- Remove GitHub's own classnames from HTML; this was causing fatal error in the page each time checkboxes were toggled
- Wrap checkbox texts in label to make them clickable
- Tell linter that jQuery is global variable
- Change how link is structured
- Change how HTML is structured
- Move link under the settings from above
appendHtml( markdownBody, '<p><strong>This branch is already merged.</strong></p>' );
}

function getLink() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of modifying the URL, we just re-generate it on each time. Feels more straightforward code-wise.

const div = document.createElement( 'div' );
function getOptionsList( options, columnWidth ) {
return `
<ul style="list-style: none; padding-left: 0; display: flex; flex-wrap: wrap;">
Copy link
Member Author

Choose a reason for hiding this comment

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

Inline CSS instead of relying on Github, because relying on those classes was causing occasional JS fatal errors.

@jetpackbot
Copy link
Collaborator

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against fa9433e

@simison simison added the [Status] Needs Review This PR is ready for review. label Mar 29, 2019
@simison
Copy link
Member Author

simison commented Mar 29, 2019

Feels like it would be more natural for this script to live in Jurassic Ninja repository? That way we could make this more generic and inject these also in WooCommerce and other repositories that Jurassic Ninja supports.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 29, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This works well for me! 🚢

@simison simison merged commit c569654 into master Mar 31, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 31, 2019
@simison simison deleted the update/jetpack-live-refactor branch March 31, 2019 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants