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

ZNTA-2487: MTMerge modal story #919

Merged
merged 9 commits into from Jun 22, 2018
Merged

ZNTA-2487: MTMerge modal story #919

merged 9 commits into from Jun 22, 2018

Conversation

@kathryngo
Copy link

kathryngo commented Jun 20, 2018

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

Modal for MT bulk merge (storybook only)

  1. Run make storybook-frontend
  2. View the Modal > MTMerge story

Please note: !important statements added to buttons in antd.less to ensure styling is not overridden by antd defaults (variables not available for these specific styles). this may be easier to fix once the react-bootstrap package is gone (which btw is twice the size of antd)

updaredmtbatchmerge

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

@zubot

This comment has been minimized.

Copy link
Member

zubot commented Jun 20, 2018

This change is Reviewable

@kathryngo kathryngo self-assigned this Jun 20, 2018
kgough added 4 commits Jun 20, 2018
…tatements added to buttons to ensure styling is not overridden by antd defaults (variables not available for these specific styles)
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jun 20, 2018

Codecov Report

Merging #919 into master will increase coverage by 0.01%.
The diff coverage is 69.23%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #919      +/-   ##
===========================================
+ Coverage     34.68%   34.7%   +0.01%     
  Complexity     5988    5988              
===========================================
  Files          1626    1627       +1     
  Lines         65143   65168      +25     
  Branches       7584    7585       +1     
===========================================
+ Hits          22598   22616      +18     
- Misses        40510   40517       +7     
  Partials       2035    2035
Impacted Files Coverage Δ Complexity Δ
server/zanata-frontend/src/app/entrypoint/index.js 0% <0%> (ø) 0 <0> (ø) ⬇️
...ntend/src/app/containers/ProjectVersion/MTMerge.js 72% <72%> (ø) 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 41b905f...c9ca5a0. Read the comment docs.

@kathryngo

This comment has been minimized.

Copy link
Author

kathryngo commented Jun 20, 2018

Change text to: "Override existing fuzzy translations with MT"

Other options:
Save as:
Translated OR Fuzzy

@efloden

This comment has been minimized.

Copy link
Member

efloden commented Jun 21, 2018

Reviewed 1 of 3 files at r1, 5 of 7 files at r2, 2 of 2 files at r3.
Review status: all files reviewed, 1 unresolved discussion (waiting on @kathryngo)


server/zanata-frontend/src/app/containers/ProjectVersion/MTMerge.js, line 71 at r3 (raw file):

            </div>
            <br />
            <CheckboxGroup options={plainOptions} value={this.state.checkedList} onChange={this.onChange} />

Could we use the ant Select with mode="tags" here instead? The CheckboxGroup would be difficult to use with so many languages


Comments from Reviewable

@kathryngo

This comment has been minimized.

Copy link
Author

kathryngo commented Jun 21, 2018

server/zanata-frontend/src/app/containers/ProjectVersion/MTMerge.js, line 71 at r3 (raw file):

Previously, efloden (Earl Floden) wrote…

Could we use the ant Select with mode="tags" here instead? The CheckboxGroup would be difficult to use with so many languages

I spoke to @seanf yesterday and it was decided that since projects have no more than 10 languages usually (and if they do they can easily be shown in the 5 column list), and that typing out each of these would be more arduous than selecting either a few, one or all checkboxes, we decided to go with the checkbox group.
Is there something about these checkboxes that would make it a better idea to use the tag activated select component?
Happy to make changes still, just wanted to tell you why I changed the UI


Comments from Reviewable

@kathryngo kathryngo merged commit 527ec1d into master Jun 22, 2018
7 of 8 checks passed
7 of 8 checks passed
codebeat 0 issues resolved and 1 introduced
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
CodeFactor No issues found.
Details
Jenkinsfile SUCCESS: Duration: 24 hr 7 min 36 sec 408 ms
Details
code-review/reviewable 8 files reviewed
Details
codecov/patch 69.23% of diff hit (target 34.68%)
Details
codecov/project 34.7% (+0.01%) compared to 41b905f
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
@kathryngo kathryngo deleted the ZNTA-2504 branch Jun 22, 2018
@aeng aeng changed the title ZNTA-2504: MTMerge modal story ZNTA-2487: MTMerge modal story Jun 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.