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

Bug 1489718 - Insert form widgets for approval flag requests instead of free-form comment text #756

Merged
merged 8 commits into from Sep 26, 2018

Conversation

4 participants
@kyoshino
Member

kyoshino commented Sep 20, 2018

Description

  • A request from the Release Management team
  • Upon selecting "?" for an approval flag, insert a temporary form that will be later converted to plaintext comment text when submitting form
  • If a template is not available, fallback to traditional plaintext comment template stored in the DB

Screenshots

Bug

Bug 1489718 - Insert form widgets for approval flag requests instead of free-form comment text

@kyoshino kyoshino added the UX label Sep 20, 2018

@kyoshino kyoshino self-assigned this Sep 20, 2018

kyoshino added some commits Sep 21, 2018

@dylanwh

lgtm but I have a question about the flags, as I'm not sure based on the data I see in prod.

# defined by the Mozilla Public License, v. 2.0.
#%]
<template class="approval-request" data-flags="approval‑mozilla‑beta approval‑mozilla‑release">

This comment has been minimized.

@dylanwh

dylanwh Sep 24, 2018

Collaborator

Are these the right flags?
What about approval-mozilla-central?

@emceeaich can you ask the stakeholder?

This comment has been minimized.

@sylvestre

sylvestre Sep 24, 2018

Contributor

I am the stakeholder for this. There is no such thing as approval-mozilla-central
this is beta, & release & esr.

I have some comments about the text/values. I will get back tomorrow.

@kyoshino kyoshino added the blocked label Sep 24, 2018

@sylvestre

This comment has been minimized.

Contributor

sylvestre commented Sep 26, 2018

Some changes in the text (discussed with ritu and ryan)

risky: high, medium, low

string changes text field should be one line

verified nightly => dunno to be removed

Needs manual test Dunno => removed too

Remove the string "for the feature/fix" (about uplift)

@sylvestre

This comment has been minimized.

Contributor

sylvestre commented Sep 26, 2018

@marco-c @jcristau if you have any comments

kyoshino added some commits Sep 26, 2018

@kyoshino

This comment has been minimized.

Member

kyoshino commented Sep 26, 2018

Here are updated screenshots 📸

@sylvestre

This comment has been minimized.

Contributor

sylvestre commented Sep 26, 2018

Sorry, my feedback wasn't perfect
"Remove the string "for the feature/fix" (about uplift)"
I just meant to remove the part of the string"for the feature/fix"
the option should still be there!

@kyoshino

This comment has been minimized.

Member

kyoshino commented Sep 26, 2018

My bad. The "List of other uplifts needed" option restored!

@sylvestre

This comment has been minimized.

Contributor

sylvestre commented Sep 26, 2018

Looks great! Let's ship it. Many thanks, i have been expecting a better solution for years!

@kyoshino kyoshino removed the blocked label Sep 26, 2018

@dylanwh dylanwh merged commit 291bb97 into mozilla-bteam:master Sep 26, 2018

6 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_info Your tests passed on CircleCI!
Details
ci/circleci: test_bmo Your tests passed on CircleCI!
Details
ci/circleci: test_sanity Your tests passed on CircleCI!
Details
ci/circleci: test_selenium Your tests passed on CircleCI!
Details
ci/circleci: test_webservices Your tests passed on CircleCI!
Details

@kyoshino kyoshino deleted the kyoshino:bug-1489718-ftc branch Sep 26, 2018

@marco-c

This comment has been minimized.

marco-c commented Sep 26, 2018

Looks great to me!
I think after we introduce regressed-by we can get rid of Bug causing the regression, but that's for the future.

@sylvestre

This comment has been minimized.

Contributor

sylvestre commented Sep 26, 2018

@pascalchevrel I think we should discuss next week to write a blog post about that :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment