{"action":"created","issue":{"url":"https://api.github.com/repos/zanata/zanata-platform/issues/404","repository_url":"https://api.github.com/repos/zanata/zanata-platform","labels_url":"https://api.github.com/repos/zanata/zanata-platform/issues/404/labels{/name}","comments_url":"https://api.github.com/repos/zanata/zanata-platform/issues/404/comments","events_url":"https://api.github.com/repos/zanata/zanata-platform/issues/404/events","html_url":"https://github.com/zanata/zanata-platform/pull/404","id":237710370,"number":404,"title":"ZNTA-1865 TM Merge Modal full version front-end","user":{"login":"efloden","id":7971187,"avatar_url":"https://avatars3.githubusercontent.com/u/7971187?v=3","gravatar_id":"","url":"https://api.github.com/users/efloden","html_url":"https://github.com/efloden","followers_url":"https://api.github.com/users/efloden/followers","following_url":"https://api.github.com/users/efloden/following{/other_user}","gists_url":"https://api.github.com/users/efloden/gists{/gist_id}","starred_url":"https://api.github.com/users/efloden/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/efloden/subscriptions","organizations_url":"https://api.github.com/users/efloden/orgs","repos_url":"https://api.github.com/users/efloden/repos","events_url":"https://api.github.com/users/efloden/events{/privacy}","received_events_url":"https://api.github.com/users/efloden/received_events","type":"User","site_admin":false},"labels":[{"id":465778273,"url":"https://api.github.com/repos/zanata/zanata-platform/labels/Work%20in%20progress","name":"Work in progress","color":"fef2c0","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":7,"created_at":"2017-06-22T01:48:53Z","updated_at":"2017-06-22T05:28:32Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/zanata/zanata-platform/pulls/404","html_url":"https://github.com/zanata/zanata-platform/pull/404","diff_url":"https://github.com/zanata/zanata-platform/pull/404.diff","patch_url":"https://github.com/zanata/zanata-platform/pull/404.patch"},"body":"JIRA issue URL: https://zanata.atlassian.net/browse/ZNTA-1865\r\n\r\n### WIP: TM Merge Modal front-end\r\n* Created project version page route at project/{projectSlug}/version/{versionSlug} \r\n* Implemented master TMMergeModal component toggled by button on project version page\r\n* Extracted ProjectVersionPanels component to display source project versions to merge\r\n* Extracted TMMatchPercentageDropdown component for match percentage selection\r\n* Extracted LanguageSelectionDropdown component for source language selection\r\n\r\n### TODO\r\n * Implement draggable list component for version priority selection\r\n * Implement debounce on project search\r\n\r\n## Reviewer tasks\r\n\r\nThe following is based on the\r\n[Zanata Development Guidelines](https://github.com/zanata/zanata-platform/wiki/Development-Guidelines).\r\nAs a reviewer, you should check the guidelines regularly for updates, and just\r\nto refresh your memory.\r\n\r\n\r\nThe reviewer can use this list for reference to make sure\r\nthey have considered all the important points.\r\n\r\nUse the checkboxes or not, whatever works best for you.\r\n\r\n - Code quality\r\n   - [ ] Code passes style check (checkstyle/eslint)\r\n   - [ ] Code is clear and concise\r\n   - [ ] UI code is internationalised\r\n   - [ ] Code has adequate comments where appropriate\r\n   - Code is in an appropriate place\r\n     - [ ] Classes are in appropriate packages\r\n     - [ ] Code fits with class's responsibilities (SRP)\r\n   - [ ] Configuration files are documented enough\r\n   - If code is removed\r\n     - [ ] No remaining code references the removed code\r\n       - [ ] No orphaned rewrite rules\r\n       - [ ] No orphaned config settings\r\n     - [ ] No remaining comments refer to the removed code\r\n     - [ ] Unused imports and libraries are removed\r\n - Testing\r\n   - [ ] New code has tests\r\n   - [ ] Changed code has tests\r\n   - [ ] All tests pass\r\n   - [ ] Test coverage stable or increased\r\n - Documentation\r\n   - [ ] New features are documented\r\n   - [ ] Docs removed for deleted features\r\n   - [ ] Docs updated for all changed features\r\n   - [ ] Developer/sysadmin docs updated if appropriate\r\n - If there are new dependencies\r\n   - [ ] Does each new dependency pass all the\r\n         [Considerations for new dependencies](https://github.com/zanata/zanata-platform/wiki/Development-Guidelines#new-technologies-and-dependencies)?\r\n\r\n\r\n----\r\n*This template can be updated in .github/PULL_REQUEST_TEMPLATE.md*\r\n\r\n<!-- Reviewable:start -->\r\n---\r\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/zanata/zanata-platform/404)\r\n<!-- Reviewable:end -->\r\n"},"comment":{"url":"https://api.github.com/repos/zanata/zanata-platform/issues/comments/310279404","html_url":"https://github.com/zanata/zanata-platform/pull/404#issuecomment-310279404","issue_url":"https://api.github.com/repos/zanata/zanata-platform/issues/404","id":310279404,"user":{"login":"davidmason","id":482026,"avatar_url":"https://avatars0.githubusercontent.com/u/482026?v=3","gravatar_id":"","url":"https://api.github.com/users/davidmason","html_url":"https://github.com/davidmason","followers_url":"https://api.github.com/users/davidmason/followers","following_url":"https://api.github.com/users/davidmason/following{/other_user}","gists_url":"https://api.github.com/users/davidmason/gists{/gist_id}","starred_url":"https://api.github.com/users/davidmason/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/davidmason/subscriptions","organizations_url":"https://api.github.com/users/davidmason/orgs","repos_url":"https://api.github.com/users/davidmason/repos","events_url":"https://api.github.com/users/davidmason/events{/privacy}","received_events_url":"https://api.github.com/users/davidmason/received_events","type":"User","site_admin":false},"created_at":"2017-06-22T05:28:32Z","updated_at":"2017-06-22T05:28:32Z","body":"\n\n\n\nReview status: 4 of 12 files reviewed at latest revision, 25 unresolved discussions, some commit checks failed.\n\n---\n\n*[server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js, line 1 at r1](https://reviewable.io:443/reviews/zanata/zanata-platform/404#-KnDG2GlTlAQFiSZvHzN:-KnDG2GlTlAQFiSZvHzO:b-t6q76f) ([raw file](https://github.com/zanata/zanata-platform/blob/5c85c8a9a3de1ec677f41e62d474f1fbff5465af/server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js#L1)):*\n> ```JavaScript\n> import React, {Component} from 'react'\n> ```\n\nThis file has several components in it, I would split it into several files. You can move this to `ProjectVersionPanel/index.js` then extract the nested components to separate files with the name of the component.\n\n---\n\n*[server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js, line 6 at r1](https://reviewable.io:443/reviews/zanata/zanata-platform/404#-KnDAvQueXVQ3t94WNKp:-KnDAvQueXVQ3t94WNKq:b2xy8r) ([raw file](https://github.com/zanata/zanata-platform/blob/5c85c8a9a3de1ec677f41e62d474f1fbff5465af/server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js#L6)):*\n> ```JavaScript\n>   Panel, Tooltip, Checkbox, ListGroup, ListGroupItem, OverlayTrigger, PanelGroup\n> }\n>   from 'react-bootstrap'\n> ```\n\nplease move this up next to the `}`\n\n---\n\n*[server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js, line 12 at r1](https://reviewable.io:443/reviews/zanata/zanata-platform/404#-KnDB8tWXVAbINxj4HBv:-KnDB8tWXVAbINxj4HBw:b7il1qm) ([raw file](https://github.com/zanata/zanata-platform/blob/5c85c8a9a3de1ec677f41e62d474f1fbff5465af/server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js#L12)):*\n> ```JavaScript\n>   <Tooltip id='tooltipreadonly'>Read only\n>   </Tooltip>\n> )\n> ```\n\nThis could probably all fit on one line.\n\n---\n\n*[server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js, line 15 at r1](https://reviewable.io:443/reviews/zanata/zanata-platform/404#-KnDBE1dHXOe1vvUhOkY:-KnDBE1dHXOe1vvUhOkZ:bfjxuft) ([raw file](https://github.com/zanata/zanata-platform/blob/5c85c8a9a3de1ec677f41e62d474f1fbff5465af/server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js#L15)):*\n> ```JavaScript\n> \n> /**\n>  * Root component for project version panels\n> ```\n\nThis is specific to the version merge modal, right? If it is not reusable elsewhere, the name and description should make it clear that the scope is limited.\n\n---\n\n*[server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js, line 20 at r1](https://reviewable.io:443/reviews/zanata/zanata-platform/404#-KnDB_dtQ6Ru_07WaBZf:-KnDB_duyHrfXapvZxgI:b-dzm1v5) ([raw file](https://github.com/zanata/zanata-platform/blob/5c85c8a9a3de1ec677f41e62d474f1fbff5465af/server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js#L20)):*\n> ```JavaScript\n>   static propTypes = {\n>     projectVersions: PropTypes.arrayOf(PropTypes.object),\n>     fromProjectVersion: PropTypes.arrayOf(PropTypes.object),\n> ```\n\nUse `PropTypes.shape()` for more specific validation of these.\n\n---\n\n*[server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js, line 25 at r1](https://reviewable.io:443/reviews/zanata/zanata-platform/404#-KnDBkk24j6llyxv8OFn:-KnDBkk24j6llyxv8OFo:b-z8j0ve) ([raw file](https://github.com/zanata/zanata-platform/blob/5c85c8a9a3de1ec677f41e62d474f1fbff5465af/server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js#L25)):*\n> ```JavaScript\n>   }\n>   render () {\n>     if (this.props.projectVersions.length <= 0) {\n> ```\n\nNegative length is impossible, so just `=== 0`.\n\n---\n\n*[server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js, line 28 at r1](https://reviewable.io:443/reviews/zanata/zanata-platform/404#-KnDBwNDdeF2xBRJY7uh:-KnDBwNDdeF2xBRJY7ui:b-18b53n) ([raw file](https://github.com/zanata/zanata-platform/blob/5c85c8a9a3de1ec677f41e62d474f1fbff5465af/server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js#L28)):*\n> ```JavaScript\n>       return <div></div>\n>     }\n>     let panels = this.props.projectVersions.map((project, index) => {\n> ```\n\nUse `const` here.\n\nOnly use `let` when you need to bind this to a different value. If we're writing good code that will be very rare.\n\n---\n\n*[server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js, line 33 at r1](https://reviewable.io:443/reviews/zanata/zanata-platform/404#-KnDCRN890JSooNrSyeR:-KnDCRN890JSooNrSyeS:b-qycg5n) ([raw file](https://github.com/zanata/zanata-platform/blob/5c85c8a9a3de1ec677f41e62d474f1fbff5465af/server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js#L33)):*\n> ```JavaScript\n>           project={project} onClick={this.props.onAllVersionCheckboxChange}\n>           fromProjectVersion={this.props.fromProjectVersion} />\n>         </h3>} key={index} eventKey={index}>\n> ```\n\nFix the layout of the header so it is more readable - either move it to a const just above, or I think you can wrap it something like this (whatever will be easier to maintain):\n\n```\n<Panel key={index} eventKey={index}\n  header={(\n    <h3>\n      <SelectAllVersionsCheckbox\n        project={project}\n        onClick={this.props.onAllVersionCheckboxChange}\n        fromProjectVersion={this.props.fromProjectVersion} />\n      </h3>)}>\n```\n\n---\n\n*[server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js, line 42 at r1](https://reviewable.io:443/reviews/zanata/zanata-platform/404#-KnDDclT2OMQGTfRqk_O:-KnDDclT2OMQGTfRqk_P:b-k6ujky) ([raw file](https://github.com/zanata/zanata-platform/blob/5c85c8a9a3de1ec677f41e62d474f1fbff5465af/server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js#L42)):*\n> ```JavaScript\n>                     fromProjectVersion={this.props.fromProjectVersion}\n>                     projectSlug={project.id} />\n>                 </ListGroupItem>\n> ```\n\nYou can avoid the `return` statement here if you want to,  but I think it is fine either way. Since there is just a single return statement in the body of the arrow function, you can replace the whole body with the returned value:\n\n```\n{project.versions.map((version, index) => (\n      <ListGroupItem className='v' key={index}>\n        <VersionMenuCheckbox version={version}\n          onClick={this.props.onVersionCheckboxChange}\n          fromProjectVersion={this.props.fromProjectVersion}\n          projectSlug={project.id} />\n      </ListGroupItem>\n...\n```\n\n---\n\n*[server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js, line 49 at r1](https://reviewable.io:443/reviews/zanata/zanata-platform/404#-KnDEXFvBL1LElE8v_VF:-KnDEXFwtUTfCOiWykbg:b1chiuu) ([raw file](https://github.com/zanata/zanata-platform/blob/5c85c8a9a3de1ec677f41e62d474f1fbff5465af/server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js#L49)):*\n> ```JavaScript\n>       )\n>     })\n>     return <PanelGroup defaultActiveKey='1' accordion>{panels}</PanelGroup>\n> ```\n\nIs this making the second item active by default, or are we using a 1-based index for this? Whichever it is we should change it if we can.\n\n---\n\n*[server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js, line 59 at r1](https://reviewable.io:443/reviews/zanata/zanata-platform/404#-KnDErt-5Z1J38MhRo4z:-KnDErt-5Z1J38MhRo5-:b-uff0sg) ([raw file](https://github.com/zanata/zanata-platform/blob/5c85c8a9a3de1ec677f41e62d474f1fbff5465af/server/zanata-frontend/src/frontend/app/components/ProjectVersionPanels.js#L59)):*\n> ```JavaScript\n> class SelectAllVersionsCheckbox extends Component {\n>   static propTypes = {\n>     project: PropTypes.object,\n> ```\n\nIt is definitely looking worth defining a constant to describe the expected shape for projects. Something like\n\n```\nconst ProjectType = PropTypes.shape({\n  // ... all the expected props in a project\n}).isRequired\n```\n\nWe do not have a central place for types like this at the moment, but we could make a module in `util` for now, so you could do `import { ProjectType } from '../util/prop-types-util.js'` anywhere that the project shape is needed in `propTypes`.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/zanata/zanata-platform/404)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"davidmason"}}