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

imprv: Show modal when enabling Textlint #4373

Merged
merged 39 commits into from
Oct 4, 2021
Merged

Conversation

stevenfukase
Copy link
Contributor

@stevenfukase stevenfukase commented Sep 24, 2021

タスク

  • #77591【GROWI】初回利用時に20MBの辞書ファイルを許可するModalを出す

内容

  • Textlintを有効にした際に、Dictionaryをダウンロードすることを警告するModalを出すようにしました。

英語(ダークモード)

Screen Shot 2021-09-28 at 15 53 22

日本語(ダークモード)

Screen Shot 2021-09-28 at 15 55 02

中国語(ライトモード)

Screen Shot 2021-09-28 at 15 56 10

ロジック

Editor画面初回マウント時

  1. CodeMirrorEditor.jsxのcomponentWillMountでDBのisTextlintEnabledを参照し、初回は空なのでEditorContainerのステートをisTextlintEnabled: nullとする
  2. Editor画面下のSwitchを押すとModalが出る。
    • 「常に許可する」にチェックを入れTextlintを有効にすると、EditorContainerのステートがisTextlintEnabled: trueになり、DB内にisTextlintEnabled: trueが追加される
    • 「常に許可する」をチェックせずTextlintを有効にすると、EditorContainerのステートのみがisTextlintEnabled: trueになる
  3. 上記のいずれの場合でも、画面を閉じなければ(CodeMirrorEditorがUnmountされなければ)再度ON/OFFをToggleしてもModalは出ない。

Editor画面2回目以降マウント時

  1. CodeMirrorEditor.jsxのcomponentWillMountでDBのisTextlintEnabledを参照する
    • 空の場合: EditorContainerのステートをisTextlintEnabled: nullに上書きされ、Editor画面下のSwitchを押すとModalが出るようになる
    • falseまたはtrueの場合: EditorContainerのステートがisTextlintEnabled: trueまたはfalseになり、Editor画面下のSwitchを押してもModalは出ない

@stevenfukase stevenfukase changed the title Imprv/77591 show modal imprv: Show modal when enabling Textlint Sep 24, 2021
@stevenfukase stevenfukase self-assigned this Sep 24, 2021
@stevenfukase stevenfukase changed the base branch from master to fix/77789-dict-path-on-server September 28, 2021 03:09
Base automatically changed from fix/77789-dict-path-on-server to master September 28, 2021 07:00

type DownloadDictModalProps = {
isModalOpen: boolean
isDontAskAgainChecked: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

isDontAskAgainChecked は不要。なぜなら「二度と聞かない」を ON にしたあとは、このモーダルは開かれることすらないから。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

対応しました

isModalOpen: boolean
isDontAskAgainChecked: boolean;
onConfirmEnableTextlint: () => void;
onCancel: () => void;
Copy link
Member

Choose a reason for hiding this comment

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

onHoge は nullable にする。呼び出すときは null チェックをする。

onConfirmEnableTextlint を呼んだ時は、isDontAskAgainChecked にあたる boolean も引数に入れる。
ただし、dont ask という否定系の名前は避ける。代案はパッといいのは思いつかないけど例えば isSkipCheckingAfterThis とか。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

対応済

isDontAskAgainChecked: boolean;
onConfirmEnableTextlint: () => void;
onCancel: () => void;
dontAskAgainCheckboxHandler: (isChecked: boolean) => void;
Copy link
Member

Choose a reason for hiding this comment

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

dontAskAgainCheckboxHandler は不要

Copy link
Contributor Author

Choose a reason for hiding this comment

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

対応済

if (editorContainer.state.isTextlintEnabled === null) {
this.setState({ isDownloadDictModalShown: true });
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

ここまではいい。

ここを通過してモーダルを開かなかった場合は、新規メソッドを作ってそれを呼ぶ。
confirmEnableTextlintHandler からもそれを呼ぶことになる。

<span className="ml-2 ml-sm-4">{this.renderConfigurationDropdown()}</span>
</div>

<DownloadDictModal
Copy link
Member

Choose a reason for hiding this comment

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

isDontAskAgainChecked が true になったら、レンダリングすらしなくていい。

Comment on lines +150 to 155
confirmEnableTextlintHandler(isSkipAskingAgainChecked) {
this.setState(
{ isSkipAskingAgainChecked, isDownloadDictModalShown: false },
() => this.toggleTextlint(),
);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.toggleTextlint()をcallbackに入れずに実行しようとするとsetStateの前に走ってしまう。
setStateはasyncではあるがPromiseを返しているわけではないのでasync awaitは使えない。
参考: StackOverflow | React setState not updating state

Comment on lines +393 to +399
{!this.state.isSkipAskingAgainChecked && (
<DownloadDictModal
isModalOpen={this.state.isDownloadDictModalShown}
onConfirmEnableTextlint={this.confirmEnableTextlintHandler}
onCancel={() => this.setState({ isDownloadDictModalShown: false })}
/>
)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

isSkipAskingAgainChecked: trueの場合、Renderしない

@yuki-takei yuki-takei merged commit c48cec3 into master Oct 4, 2021
@yuki-takei yuki-takei deleted the imprv/77591-show-modal branch October 4, 2021 09:41
@github-actions github-actions bot mentioned this pull request Oct 4, 2021
maow89126 added a commit that referenced this pull request Oct 7, 2021
…t/77515-77833-arrange-component-temporaly

* feat/77515-display-search-result-with-snippet: (47 commits)
  revert
  wip show snippet
  wip fix presentation of snippets
  send search result
  make the first SlackAppIntegration primary
  replace add-app-to-this-channel image
  fix: New user is created on SAML login even if attribute-based login control failure (#4422)
  fix: Redirected to apiv3 endpoint when guest mode is enabled (#4443)
  imprv: Show modal when enabling Textlint (#4373)
  imprv: Slackbot reaction to user (#4442)
  update dependabot setting
  fix(slackbot): Stop auto-join to channels with middlewarer (#4424)
  Bump version
  Release v4.4.7
  imprv: Slackbot search (#4420)
  update release-slackbot-proxy.yml
  Bump version
  bump version for slackbot-proxy
  add npm script for bump-versions
  fix(slackbot): Sync permission when data stored is not enough (#4417)
  ...

# Conflicts:
#	packages/app/src/components/SearchPage/SearchResult.jsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants