Organise utilities #59
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
remo5000
requested changes
May 29, 2018
| </div> | ||
| </div> | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| const controlButton = ( |
There was a problem hiding this comment.
Nice, can this be applied to Editor as well? I think it still uses genericButton
There was a problem hiding this comment.
Sounds good (and DRY), but how do you wanna go about it?
- Change
Editorto have the same functioncontrolButton - .Export
controlButtonfrom here and import it intoEditor - Abstract
controlButtoninto a separate file withinworkspace - Abstract
controlButtoninto a new/existing file underutils
I'm leaning towards (2) tbh
There was a problem hiding this comment.
Since controlButton does not originate from here, I think abstracting into the utils folder is better. If there was something that belongs to ReplControl, it would make more sense to export.
remo5000
approved these changes
May 30, 2018
- utils is a new directory under ./src/ holding misc modules that don't seem to fit anywhere else. - createStore was excluded from utils since the store is a core part of our react/redux app, and so it does make sense as a standalone module in the root ./src/ directory - Also fixed #37 unnecessary yield undefined.
Also avoided variable name negation by renaming its argument 'notMinimal' -> 'minimal'. Refactor ReplControl to list declarations in decreasing levels of abstraction. (addresses review in PR #35)
- Add a mockTypeError in mocks for situations that require a mock SourceError of some kind.
Also fixed typescript type errors on yarn start.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Features
notification.tsandregisterServiceWorker.tsinto their own utility directorygenericButton->controlButtonaccording to PR Add Source §2 #35yield undefinedaccording to issue Follow Up on Review of PR35 #37ReplControl.tsxto list declarations in decreasing levels of abstractionRepl.tsxfrom 67% to 100%Issues fixed