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

fix(ZNTA-2313): update admin rejection panel #672

Merged
32 commits merged into from
Feb 26, 2018
Merged

fix(ZNTA-2313): update admin rejection panel #672

32 commits merged into from
Feb 26, 2018

Conversation

ghost
Copy link

@ghost ghost commented Jan 18, 2018

JIRA issue URL: https://zanata.atlassian.net/browse/ZNTA-2313

Removed the accordion with the text input rows. This is kind of confusing since these can't be used as a starting point without copying the text so the examples have been included as text under the description.

The toggle editable button is removed as it isn't necessary. If admins want to change the criteria they can just edit it, and if they aren't happy with their changes they shouldn't save them.

Also fixed padding and spacing in the rows and added breadcrumbs as requested.

QA

Please test the functionality of the admin Reject Criteria page in frontend

Initial page

1

Add first criteria

2

Add second criteria

3

Modal - criteria added

reject-withcriteria

Modal - no criteria added

nocriteria

Checklist

  • JIRA link
  • Check target branch
  • Make sure all commit statuses are green or otherwise documented reasons to ignore
  • QA needs to evaluate against the JIRA ticket
  • Changed files and commits make sense for this PR

See Zanata Development Guidelines more for information.


This template can be updated in .github/PULL_REQUEST_TEMPLATE.md


This change is Reviewable

@ghost ghost added the Work in progress label Jan 18, 2018
@codecov
Copy link

codecov bot commented Jan 19, 2018

Codecov Report

Merging #672 into master will increase coverage by <.01%.
The diff coverage is 85.71%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #672      +/-   ##
============================================
+ Coverage     35.24%   35.25%   +<.01%     
  Complexity     5813     5813              
============================================
  Files          1586     1587       +1     
  Lines         62761    62768       +7     
  Branches       7297     7298       +1     
============================================
+ Hits          22123    22130       +7     
  Misses        38688    38688              
  Partials       1950     1950
Impacted Files Coverage Δ Complexity Δ
.../editor/components/RejectTranslationModal/index.js 88.88% <100%> (ø) 0 <0> (ø) ⬇️
...ctTranslationModal/RejectTranslationModalNoCrit.js 100% <100%> (ø) 0 <0> (?)
...zanata-frontend/src/app/containers/Admin/Review.js 31.03% <60%> (-4.45%) 0 <0> (ø)
...rontend/src/app/components/RejectionsForm/index.js 80% <75%> (+0.45%) 0 <0> (ø) ⬇️
...main/java/org/zanata/servlet/UrlRewriteConfig.java 79.24% <0%> (-0.2%) 4% <0%> (ø)
server/zanata-frontend/src/app/containers/Root.js 50% <0%> (ø) 0% <0%> (ø) ⬇️
.../zanata-frontend/src/app/containers/Admin/index.js 42.85% <0%> (ø) 0% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e38d758...ffc85ae. Read the comment docs.

@ghost ghost added the Ignore CodeCov label Jan 19, 2018
@ghost ghost changed the title fix(ZNTA-2313) WIP: update admin rejection panel fix(ZNTA-2313): update admin rejection panel Jan 19, 2018
@ghost ghost requested a review from huangp January 19, 2018 05:50
@ghost ghost self-assigned this Jan 19, 2018
@ghost ghost requested review from efloden and removed request for huangp January 21, 2018 23:09
@efloden
Copy link
Member

efloden commented Jan 22, 2018

Reviewed 1 of 3 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@efloden
Copy link
Member

efloden commented Jan 22, 2018

:lgtm:

@ghost ghost added the On QA label Jan 22, 2018
@djansen-redhat
Copy link
Contributor

djansen-redhat commented Jan 22, 2018

  • The "editable toggle" I believe actually referred to the use of the option in the editor - in that it means text in the rejection field will be appended to the criteria reason. If not editable, then that text will be ignored.
  • No indication of save success
  • 500 Server error on attempting to delete a used criteria (!?) - ZNTA-2359
  • No field max sizing, 500 server error on too much text - ZNTA-2361
  • No title/tooltip for 'Delete criteria'
  • TextArea can be resized beyond the parent container, resulting in clipped buttons. Should just be a TextField anyhow.
  • Can save a criteria without text - ZNTA-2360
  • The parentheses seem unnecessary, it could just be eg Translation Errors: terminology, mistranslated, etc
  • Not sure if it matters at all, but the design is dissimilar to the "centred column" of the other admin pages
  • Deletion confirmation?

Not directly related:

  • The amount of text that can be entered causes the dropdown in the editor to extend well past the page
  • Empty text still shows in the dropdown (edit: fixed in ZNTA-2360)

@ghost
Copy link
Author

ghost commented Jan 23, 2018

The "editable toggle" I believe actually referred to the use of the option in the editor - in that it means text in the rejection field will be appended to the criteria reason. If not editable, then that text will be ignored.

  • is this even necessary? Seems to overcomplicate the UI and surely the field can be ignored if there is not text and any text in the rejection field will be appended anyways.

No indication of save success

  • use Alert - success to do this
  • Kathryn to add example to storybook

500 Server error on attempting to delete a used criteria (!?)

  • good luck with that one

No field max sizing, 500 server error on too much text

  • Kathryn to fix

No title/tooltip for 'Delete criteria'

  • Kathryn to fix

TextArea can be resized beyond the parent container, resulting in clipped buttons. Should just be a TextField anyhow.

  • Kathryn to fix

Can save a criteria without text

  • another one for the engineers

The parentheses seem unnecessary, it could just be eg Translation Errors: terminology, mistranslated, etc

  • Kathryn to fix

Not sure if it matters at all, but the design is dissimilar to the "centred column" of the other admin pages

  • more content, needs a wider view

Deletion confirmation?

  • Kathryn to fix

Not directly related:

The amount of text that can be entered causes the dropdown in the editor to extend well past the page

  • Kathryn to fix

Empty text still shows in the dropdown

  • Unsure what I can do about this one

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@ghost ghost added the Work in progress label Jan 23, 2018
@ghost ghost removed the Awaiting review label Feb 20, 2018
@ghost
Copy link
Author

ghost commented Feb 20, 2018

Review status: 8 of 10 files reviewed at latest revision, 1 unresolved discussion.


server/zanata-frontend/src/app/editor/components/RejectTranslationModal/RejectTranslationModalNoCrit.js, line 6 at r9 (raw file):

Previously, efloden (Earl Floden) wrote…

Some unused imports and variables appear to cause the build failure in this file: (Dropdown, Icon, key, className, onHide, isOpen etc in the build logs)

Done.


Comments from Reviewable

@efloden
Copy link
Member

efloden commented Feb 21, 2018

Reviewed 1 of 1 files at r10, 1 of 1 files at r11.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

const commentToggle = isAdminMode ? (
<FormGroup controlId='formInlineEditable'>
<ControlLabel>Editable</ControlLabel><br />
const editableToggle = isAdminMode ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

eslint error: editableToggle is defined but not used

@ghost
Copy link
Author

ghost commented Feb 22, 2018

Review status: 5 of 9 files reviewed at latest revision, 1 unresolved discussion.


server/zanata-frontend/src/app/components/RejectionsForm/index.js, line 125 at r12 (raw file):

Previously, djansen-redhat (Damian Jansen) wrote…

eslint error: editableToggle is defined but not used

Done.


Comments from Reviewable

@spathare21
Copy link
Member

@kathryngo

screenshot from 2018-02-22 12-13-05

  • 400 bad request on adding review criteria which contains lot of text (there should be limitation of characters in review criteria text box)

screenshot from 2018-02-22 12-13-17

@djansen-redhat
Copy link
Contributor

Reviewed 2 of 4 files at r12, 1 of 2 files at r13, 2 of 2 files at r14.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@djansen-redhat
Copy link
Contributor

@spathare21 at this present moment the Operation Failed notice can stay. We can consider a fix later.
@kathryngo @efloden agree?

@ghost
Copy link
Author

ghost commented Feb 23, 2018

@djansen-redhat agreed

@ghost
Copy link
Author

ghost commented Feb 23, 2018

@djansen-redhat ready to verify?

@spathare21
Copy link
Member

spathare21 commented Feb 23, 2018

@djansen-redhat ok thanks then verifying it again if anything is missing

@spathare21 spathare21 added Verified and removed On QA labels Feb 23, 2018
@ghost
Copy link
Author

ghost commented Feb 25, 2018

will merge after full func. tests are run

@djansen-redhat
Copy link
Contributor

Reviewed 1 of 2 files at r15.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@ghost ghost merged commit ecc0cab into master Feb 26, 2018
@ghost ghost deleted the ZNTA-2313 branch February 26, 2018 01:00
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants