-
Notifications
You must be signed in to change notification settings - Fork 73
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
Delete screenshot #853
Delete screenshot #853
Conversation
e635cfe
to
c987361
Compare
@@ -5,9 +5,12 @@ class GitBlameScreenshotViewerActions { | |||
constructor() { | |||
this.generateActions( | |||
"open", | |||
"close", | |||
"closeScreenshotsViewer", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the rational between renaming "close"? should "open" be renamed too then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're now using some actions in multiple sources. closeScreenshotsViewer is being listened by GitBlameStore, which is used to store the data for the textUnitInfo modal. I had to rename this action so it wouldn't get mixed up with GitBlameStore's close method. The open action is not being called by anybody else, but I agree it's better to keep them both in the same standard.
@@ -165,7 +163,7 @@ class BranchesSearchResults extends React.Component { | |||
} | |||
</Col> | |||
<Col md={2}> | |||
{this.renderOpenModalScreenshotButton(branchStatistic)} | |||
{this.renderOpenModalScreenshotButton(branchStatistic, !this.hasScreenshot(branchStatistic))} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why that change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After removing the last screenshot, the branches table must be updated and the 'open screenshots' button must be disabled. I thought if we kept the isDisabled logic inside the function, react wouldn't capture the change and therefore wouldn't re-render so instinctively I placed it in the render method but I just checked leaving it in the function and it works 😅 I'll change it back
webapp/src/main/resources/public/js/components/workbench/Workbench.js
Outdated
Show resolved
Hide resolved
webapp/src/main/resources/public/js/components/workbench/Workbench.js
Outdated
Show resolved
Hide resolved
c83aff1
to
5ad1be0
Compare
- Available from ScreenshotViewerModal - Workbench page (TextUnitInfoModal > View Screenshots) - Branches page (See Screenshots button in branches search result table) - Frontend: Separate ScreenshotPageDataSource.js and ScreenshotsViewerDataSource.js
5ad1be0
to
171a8fb
Compare
- Available from ScreenshotViewerModal - Workbench page (TextUnitInfoModal > View Screenshots) - Branches page (See Screenshots button in branches search result table) - Frontend: Separate ScreenshotPageDataSource.js and ScreenshotsViewerDataSource.js
- Available from ScreenshotViewerModal - Workbench page (TextUnitInfoModal > View Screenshots) - Branches page (See Screenshots button in branches search result table) - Frontend: Separate ScreenshotPageDataSource.js and ScreenshotsViewerDataSource.js
…terest/mojito into delete_screenshot
…terest/mojito into delete_screenshot
No description provided.