POC: pulling redux up out of v1 keypad #480
Conversation
|
handeyeco
changed the title
|
Size Change: +38 B (0%) Total Size: 650 kB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
I worry that we're swapping one issue for another with the move from Redux to prop drilling. I think that much of the state that is currently being store at the top-level could be moved into some of the components. For instance, why do we need to know about the state of the button echos outside of the buttons themselves. Potentially we could also have the popover state live closer to where it's used.
| heightPx: number; | ||
| widthPx: number; |
There was a problem hiding this comment.
style nit: we don't normally put suffix on height and width. The assumption that React uses is that if the value doesn't contain a unit of measure that the value is assumed to be in pixels.
| heightPx: state.layout.buttonDimensions.heightPx, | ||
| widthPx: state.layout.buttonDimensions.widthPx, |
There was a problem hiding this comment.
suggestion: instead of passing this around as separate values everywhere, maybe combine these into a data structure, e.g. Dimensions that can be passed around.
| interface ReduxProps { | ||
| active?: boolean; | ||
| active: boolean; | ||
| extraKeys?: ReadonlyArray<string>; | ||
| keypadType?: KeypadType; | ||
| layoutMode?: keyof typeof LayoutModes; | ||
| navigationPadEnabled?: boolean; | ||
| currentPage: number; | ||
| cursorContext?: CursorContext; | ||
| dynamicJumpOut: boolean; | ||
| paginationEnabled: boolean; | ||
| echoes: ReadonlyArray<Echo>; | ||
| popover: Popover | null; | ||
| heightPx: number; | ||
| widthPx: number; | ||
| gestureManager: GestureManager; | ||
| gestureFocus: Key | null; | ||
| } |
There was a problem hiding this comment.
question: is the goal to eventually get rid of this use of redux as well?
| heightPx: number; | ||
| widthPx: number; |
There was a problem hiding this comment.
suggestion: people might think that this specifies the width/height of the keypad when in reality it's specifying the dimensions of the buttons. Maybe these could be group together and the prop renamed, e.g.
buttonDimensions: Dimensions,
or
buttonSize: Size,
## Summary: As part of [this PR](#480) I started to understand `touchable-keypad-button`. I'm not sure we'll want to merge that PR, so just in case I decided to make this PR that takes this opportunity to type the component. Part of: LC-746 ## Test plan: Logic shouldn't change, only types Author: handeyeco Reviewers: handeyeco, jeremywiebe, kevinbarabash Required Reviewers: Approved By: jeremywiebe Checks: ✅ finish_coverage, ✅ Publish npm snapshot (ubuntu-latest, 16.x), ✅ Extract i18n strings (ubuntu-latest, 16.x), ✅ Cypress Coverage (ubuntu-latest, 16.x), ✅ Lint, Typecheck, and Test (ubuntu-latest, 16.x), ✅ Check builds for changes in size (ubuntu-latest, 16.x), ✅ Jest Coverage (ubuntu-latest, 16.x), ✅ gerald, ✅ Check for .changeset file (ubuntu-latest, 16.x), ✅ .github/dependabot.yml Pull Request URL: #483
|
We decided to not use Redux in v2 keypad |
Summary:
This is a WIP/POC of what things might look like if we were to pull Redux up out of the v1 keypad.
It's not working as-is and at this point I think it's a dead end (and a mistake on my part to have pursued it).
Thoughts:
I think we could clean this up quite a bit using Context/Hooks, but given that this all seems to be for the Keypad and we're going to look at replacing the Keypad with v2...I'm not sure we need to keep trying to refactor this.
Issue: LC-729
Test plan: