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

Delete screenshot #853

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

montedonio-pinterest
Copy link
Contributor

No description provided.

@@ -5,9 +5,12 @@ class GitBlameScreenshotViewerActions {
constructor() {
this.generateActions(
"open",
"close",
"closeScreenshotsViewer",
Copy link
Collaborator

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?

Copy link
Contributor Author

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))}
Copy link
Collaborator

Choose a reason for hiding this comment

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

why that change?

Copy link
Contributor Author

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

@montedonio-pinterest montedonio-pinterest force-pushed the delete_screenshot branch 2 times, most recently from c83aff1 to 5ad1be0 Compare August 30, 2022 13:36
    - 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
    - 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants