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

Created warning box when finding and replacing characters #1209

Merged
merged 10 commits into from
Jun 28, 2021

Conversation

JacobHaimes
Copy link
Contributor

@JacobHaimes JacobHaimes commented Jun 24, 2021

Added a new warning box when clicking the apply button under find and replace. Warning displays the found characters and the new replacing characters so the user has a better grasp as to what they are modifying. Future work would be to include the number of occurrences in the warning box.


This change is Reviewable

@JacobHaimes JacobHaimes self-assigned this Jun 24, 2021
@JacobHaimes JacobHaimes linked an issue Jun 24, 2021 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2021

Codecov Report

Merging #1209 (f2103a8) into master (189be31) will increase coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1209      +/-   ##
==========================================
+ Coverage   45.33%   45.35%   +0.01%     
==========================================
  Files         266      267       +1     
  Lines        8110     8114       +4     
  Branches      528      528              
==========================================
+ Hits         3677     3680       +3     
- Misses       4017     4018       +1     
  Partials      416      416              
Flag Coverage Δ
backend 58.43% <ø> (+0.07%) ⬆️
frontend 33.49% <0.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...erDetail/FindAndReplace/CharacterReplaceDialog.tsx 0.00% <0.00%> (ø)
...rDetail/FindAndReplace/FindAndReplaceComponent.tsx 0.00% <0.00%> (ø)
Backend/Helper/DuplicateFinder.cs 85.80% <0.00%> (+1.93%) ⬆️

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 189be31...f2103a8. Read the comment docs.

@johnthagen
Copy link
Collaborator

@JacobHaimes There is a small typo in the PR title

reaplacing

Since GitHub puts the title into the final squashed commit message it will be saved in history. Could you edit the PR title and fix the typo?

@johnthagen
Copy link
Collaborator


src/goals/CharInventoryCreation/components/CharacterDetail/FindAndReplace/CharacterReplaceDialog.tsx, line 1 at r2 (raw file):

Future work would be to include the number of occurrences in the warning box.

Do we want to open a follow on issue to track this?

@johnthagen
Copy link
Collaborator


src/goals/CharInventoryCreation/components/CharacterDetail/FindAndReplace/CharacterReplaceDialog.tsx, line 2 at r2 (raw file):

import {
  Button,

Since this PR closes #1111, the flow we use it to put "Closes #1111" in the description of the PR. When this PR is merged, GitHub will automatically close that issue then.

@johnthagen
Copy link
Collaborator


src/goals/CharInventoryCreation/components/CharacterDetail/FindAndReplace/CharacterReplaceDialog.tsx, line 31 at r2 (raw file):

      aria-describedby="alert-dialog-description"
    >
      <DialogTitle id="alert-dialog-title">

This is just an observation @imnasnainaec would probably know best about theming. But for a warning dialogue like this I would personally expect some red or orange color to help highlight the gravity of this warning.

@johnthagen
Copy link
Collaborator


src/goals/CharInventoryCreation/components/CharacterDetail/FindAndReplace/FindAndReplaceComponent.tsx, line 106 at r2 (raw file):

            );
          }}
        ></CharacterReplaceDialog>

Since this tag is empty, it can be simplified to:

        <CharacterReplaceDialog
          open={this.state.warningDialogOpen}
          dialogFindValue={this.state.findValue}
          dialogReplaceValue={this.state.replaceValue}
          handleCancel={() => {
            this.setState(() => ({
              warningDialogOpen: false,
            }));
          }}
          handleAccept={() => {
            this.setState(() => ({
              warningDialogOpen: false,
            }));
            this.props.findAndReplace(
              this.state.findValue,
              this.state.replaceValue
            );
          }}
        />

Copy link
Collaborator

@johnthagen johnthagen left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @JacobHaimes)

@johnthagen
Copy link
Collaborator


src/goals/CharInventoryCreation/components/CharacterDetail/FindAndReplace/CharacterReplaceDialog.tsx, line 2 at r2 (raw file):

Previously, johnthagen wrote…

Since this PR closes #1111, the flow we use it to put "Closes #1111" in the description of the PR. When this PR is merged, GitHub will automatically close that issue then.

See: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

Copy link
Collaborator

@imnasnainaec imnasnainaec left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @JacobHaimes)


src/goals/CharInventoryCreation/components/CharacterDetail/FindAndReplace/CharacterReplaceDialog.tsx, line 31 at r2 (raw file):

Previously, johnthagen wrote…

This is just an observation @imnasnainaec would probably know best about theming. But for a warning dialogue like this I would personally expect some red or orange color to help highlight the gravity of this warning.

Since we don't have that in our other warning dialogues, let's open that as a separate issue.

@johnthagen
Copy link
Collaborator


src/goals/CharInventoryCreation/components/CharacterDetail/FindAndReplace/CharacterReplaceDialog.tsx, line 31 at r2 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Since we don't have that in our other warning dialogues, let's open that as a separate issue.

#1212

@JacobHaimes JacobHaimes changed the title Created warning box when finding and reaplacing characters Created warning box when finding and replacing characters Jun 25, 2021
Copy link
Contributor Author

@JacobHaimes JacobHaimes left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @JacobHaimes and @johnthagen)


src/goals/CharInventoryCreation/components/CharacterDetail/FindAndReplace/CharacterReplaceDialog.tsx, line 1 at r2 (raw file):

Previously, johnthagen wrote…

Future work would be to include the number of occurrences in the warning box.

Do we want to open a follow on issue to track this?

It might be good to make one if the number of occurrences is needed in the warning.


src/goals/CharInventoryCreation/components/CharacterDetail/FindAndReplace/FindAndReplaceComponent.tsx, line 106 at r2 (raw file):

Previously, johnthagen wrote…

Since this tag is empty, it can be simplified to:

        <CharacterReplaceDialog
          open={this.state.warningDialogOpen}
          dialogFindValue={this.state.findValue}
          dialogReplaceValue={this.state.replaceValue}
          handleCancel={() => {
            this.setState(() => ({
              warningDialogOpen: false,
            }));
          }}
          handleAccept={() => {
            this.setState(() => ({
              warningDialogOpen: false,
            }));
            this.props.findAndReplace(
              this.state.findValue,
              this.state.replaceValue
            );
          }}
        />

Done.

Copy link
Collaborator

@imnasnainaec imnasnainaec left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @JacobHaimes and @johnthagen)


src/goals/CharInventoryCreation/components/CharacterDetail/FindAndReplace/CharacterReplaceDialog.tsx, line 1 at r2 (raw file):

Previously, JacobHaimes wrote…

It might be good to make one if the number of occurrences is needed in the warning.

Yes, please open a new issue for that.


src/goals/CharInventoryCreation/components/CharacterDetail/FindAndReplace/CharacterReplaceDialog.tsx, line 13 at r4 (raw file):

interface ReplaceDialogProps {
  open: boolean;
  textId?: string;

Delete unused propertytextId?: string;

Copy link
Contributor Author

@JacobHaimes JacobHaimes left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @imnasnainaec, @JacobHaimes, and @johnthagen)


src/goals/CharInventoryCreation/components/CharacterDetail/FindAndReplace/CharacterReplaceDialog.tsx, line 13 at r4 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Delete unused propertytextId?: string;

Done.

Copy link
Collaborator

@imnasnainaec imnasnainaec left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @JacobHaimes and @johnthagen)


src/goals/CharInventoryCreation/components/CharacterDetail/FindAndReplace/FindAndReplaceComponent.tsx, line 83 at r5 (raw file):

            this.setState(() => ({
              warningDialogOpen: true,
            }));

A function is only needed inside setState when you are using a value in the previous state, such as this.setState((prevState)=>({count: prevState.count+1}). For this case (and likewise below), you can use an object instead of a function. this.setState({warningDialogOpen: true)).

Copy link
Contributor Author

@JacobHaimes JacobHaimes left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @imnasnainaec, @JacobHaimes, and @johnthagen)


src/goals/CharInventoryCreation/components/CharacterDetail/FindAndReplace/FindAndReplaceComponent.tsx, line 83 at r5 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…
            this.setState(() => ({
              warningDialogOpen: true,
            }));

A function is only needed inside setState when you are using a value in the previous state, such as this.setState((prevState)=>({count: prevState.count+1}). For this case (and likewise below), you can use an object instead of a function. this.setState({warningDialogOpen: true)).

Done.

Copy link
Collaborator

@imnasnainaec imnasnainaec left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r4, 1 of 1 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @JacobHaimes and @johnthagen)

Copy link
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @JacobHaimes and @johnthagen)

@jasonleenaylor jasonleenaylor dismissed johnthagen’s stale review June 28, 2021 21:49

Review questions addressed.

Copy link
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @JacobHaimes and @johnthagen)

Copy link
Contributor Author

@JacobHaimes JacobHaimes left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@JacobHaimes JacobHaimes left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johnthagen)


src/goals/CharInventoryCreation/components/CharacterDetail/FindAndReplace/CharacterReplaceDialog.tsx, line 2 at r2 (raw file):

Previously, JacobHaimes wrote…

Will do

Done.

@jasonleenaylor jasonleenaylor merged commit 221205b into master Jun 28, 2021
@jasonleenaylor jasonleenaylor deleted the character-replace-warning branch June 28, 2021 21:58
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.

Replacing a character with nothing is too easy
5 participants