-
-
Notifications
You must be signed in to change notification settings - Fork 148
Add regex presets menu #731
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
base: main
Are you sure you want to change the base?
Conversation
killergerbah
left a comment
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.
Thanks for contributing!
This is close, but a new setting is not necessary to store the selected preset. Please use useState instead.
common/components/SettingsForm.tsx
Outdated
| import NoteTypeTutorialBubble from './NoteTypeTutorialBubble'; | ||
|
|
||
| const regexPresetsList = [ | ||
| { name: 'No preset', regex: '' }, |
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.
These strings need to be localizable.
|
Added the possibility to localize and changed the logic to use state instead of having a new setting. |
| onChange={(event) => { | ||
| handleSettingChanged('subtitleRegexFilter', event.target.value); | ||
| if (selectedRegexPresetName !== regexPresetsList[0].name) { | ||
| setSelectedRegexPresetName(regexPresetsList[0].name); |
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.
It's risky to update state from inside the render function. You should consider using useEffect instead.
Also, ideally if the user types in one of the presets manually the correct one gets selected. This code is selecting the first preset no matter what.
In regard to #237
Localization should probably be added for each preset name don't know how to do.
Support could be added for the (.)\n+(?!-)(.) indicated in the guide in order to change the "regex filter text replacement field" when this regex is selected.
Additional features:
Maybe support for multiples custom preset made by the user?
Note: First pull request
If anything is wrong feel free to explain