Improve pull request template proposal #56756
Conversation
Looking through Kibana PRs many currently ignore the check boxes or just delete them. The goal here is to make the process easier, delete some unused options and align the checkboxes with what's most useful. We've had some discussions on the Uptime team about ways the PR template could be potentially improved. These changes are based on an extended discussion we had on the topic. We'd love to hear if other teams would be OK with these recommended changes. The changes here are: * Allow authors to just delete unnecessary items rather than strike through. Adding all the `~~` in markdown is painful * Remove the unnecessary checkbox for release notes, the build bot catches this and blocks merges without that being set. * Add a checkbox for testing in various browser sizes / mobile responsive devices * Remove the IE 11 testing checkbox. This is too heavy for every PR, we skip this check on our PRs and think it's likely that other teams do as well.
andrewvc
added
discuss
Team:Core
|
Pinging @elastic/uptime (Team:uptime) |
|
Pinging @elastic/kibana-platform (Team:Platform) |
|
I have no problems with any of these changes, but I think the Platform team is not the right audience to review these. This checklist is geared towards UI and feature work, which our team does very little of. Maybe @elastic/kibana-app-arch or @elastic/kibana-app would have more of an opinion? |
Worth noting, we made an intentional decision in the original PR to keep IE11 on the checklist, specifically because most people just ignore it. Not saying we shouldn't revisit it, but just wanted to point this out. IE11 is still officially supported on the browser support matrix, and while I won't pretend I have faithfully tested 100% of PRs against IE11, I'm glad that I have to make the uncomfortable choice to remove/ignore it if I choose not to test it. Keeps me honest 🙂 cc @elastic/kibana-design and @elastic/kibana-qa who may have further input |
|
To add to @lukeelmers' point, it's not specifically just IE11 either. But cross-browser means Chrome, Safari and Firefox. Firefox having some unusual rendering issues not seen in Chrome sometimes. I think it's very important to keep as a bullet point mainly as a reminder for those working directly on the UI that it just needs a glance in these other browsers. |
|
Is that checkbox really helping us achieve our goal of cross-browser compatibility if it's often ignored? On uptime it certainly isn't, but if it's genuinely helping other teams I'm OK with leaving it on. It feels like having the first checkbox be something most people ignore devalues the rest of the checklist IMHO. Are there other ways we could achieve cross browser compat? Perhaps testing around feature freezes for instance? |
|
I think that's an applicable question to all of the checklist items. Whether or not they're read / adhered to is up to the author. Really it just serves as a reminder that it's important enough to check and gives reviewers (and specifically the design team) a reference to say, you need to check this. If there's a worry that it might stop authors from reading further down the list, we can move it to be the last bullet point. |
|
@lukeelmers @cchaos would you be opposed to moving the IE11 checkbox to the bottom of the list? From a psychological perspective it's the most painful of the boxes, and the least fulfilled. By moving it to the bottom we might be able to nudge more people to start on the list, and perhaps by the time they get there it will seem less scary. |
|
Yep, that works for me! |
|
Is there any remaining opposition to merging this now that the IE 11 check box has been left in? Please approve this PR if you're good with merging it, if you think we need to address more issues please leave a comment! |
andrewvc
added
release_note:skip
There was a problem hiding this comment.
That works for me and was my only major concern. Thanks for making that update @andrewvc!
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* master: (23 commits) Properly handle password change for users authenticated with provider other than `basic`. (elastic#55206) Improve pull request template proposal (elastic#56756) Only change handlers as the element changes (elastic#56782) [SIEM][Detection Engine] Final final rule changes (elastic#56806) [SIEM][Detection Engine] critical blocker, wrong ilm policy, need to match beats ilm policy Move ui/agg_types in to shim data plugin (elastic#56353) [SIEM] Fixes Signals count spinner (elastic#56797) [docs] Update upgrade version path (elastic#56658) [Canvas] Use unique Id for Canvas Embeddables (elastic#56783) [Rollups] Adjust max width for job detail panel (elastic#56674) Prevent http client from converting our form data (elastic#56772) Disable creating alerts client instances when ESO plugin is using an ephemeral encryption key (elastic#56676) Bumps terser-webpack-plugin to 2.3.4 (elastic#56662) Advanced settings component registry ⇒ kibana platform plugin (elastic#55940) [Endpoint] EMT-67: add kql support for endpoint list (elastic#56328) Implement UI for Create Alert form (elastic#55232) Fix: Filter pill base coloring (elastic#56761) fix open close signal on detail page (elastic#56757) [Search service] Move loadingCount to sync search strategy (elastic#56335) Rollup TSVB integration: Add test and fix warning text (elastic#56639) ...
Looking through Kibana PRs many currently ignore the check boxes or just delete them. The goal here is to make the process easier, delete some unused options and align the checkboxes with what's most useful. We've had some discussions on the Uptime team about ways the PR template could be potentially improved. These changes are based on an extended discussion we had on the topic. We'd love to hear if other teams would be OK with these recommended changes. The changes here are: * Allow authors to just delete unnecessary items rather than strike through. Adding all the `~~` in markdown is painful * Remove the unnecessary checkbox for release notes, the build bot catches this and blocks merges without that being set. * Add a checkbox for testing in various browser sizes / mobile responsive devices * Move IE checkbox to the bottom of the list since it's seldom checked and makes the checklist seem daunting
…ention from PR template [skip ci] (#70486) (#70789) * Improve pull request template proposal (#56756) Looking through Kibana PRs many currently ignore the check boxes or just delete them. The goal here is to make the process easier, delete some unused options and align the checkboxes with what's most useful. We've had some discussions on the Uptime team about ways the PR template could be potentially improved. These changes are based on an extended discussion we had on the topic. We'd love to hear if other teams would be OK with these recommended changes. The changes here are: * Allow authors to just delete unnecessary items rather than strike through. Adding all the `~~` in markdown is painful * Remove the unnecessary checkbox for release notes, the build bot catches this and blocks merges without that being set. * Add a checkbox for testing in various browser sizes / mobile responsive devices * Move IE checkbox to the bottom of the list since it's seldom checked and makes the checklist seem daunting * Remove IE11 mention from PR template [skip ci] (#70486) * Remove IE11 mention from PR template * Add link to browser matrix Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Andrew Cholakian <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Looking through Kibana PRs many currently ignore the check boxes or just delete them. The goal here is to make the process easier, delete some unused options and align the checkboxes with what's most useful. We've had some discussions on the Uptime team about ways the PR template could be potentially improved. These changes are based on an extended discussion we had on the topic. We'd love to hear if other teams would be OK with these recommended changes.
The changes here are:
~~in markdown is painful