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.
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
Fix/address page state navigation 2 #36770
Fix/address page state navigation 2 #36770
Changes from 73 commits
cd8ca07
1284e59
963cf67
1305561
16ba084
0f83cff
0951d5d
3b88d46
598c7c6
4a21636
312d0c6
8a293b5
c946cbb
144eea9
d2848d3
17f2aab
9fb4f72
d6a643c
080285a
9bb88ee
5e38447
470ec03
8f28580
7be20b3
428e48c
7cd8b6f
5a02b71
ec0eb6f
07f6f21
77f236c
877740b
6731502
a6faef4
7db144b
c0c1c6b
7b15308
21b215a
0fd833f
8c59969
397278d
4a8d6d6
5d223d6
3a3c767
c56f8d5
470be54
1670b66
5d2ff1e
18717ea
3ba97a5
0dc7f49
57bcee2
9ffea05
d25904c
81f452c
dc35055
a764d21
78afcd5
ecc3953
713a91c
9a79421
2a76bed
49f2e46
15c54b2
58d92f1
af6c3c5
24cbec3
2619a29
f48a7fc
4efccc0
b9fe092
5f0cc47
3585296
639a646
7e81480
3f87bf2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
This file was deleted.
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.
NAB
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.
I am trying to understand the usages of state from URL
stateFromUrl
and state as a direct propstateCode
.Here the title is calculated form the stateCode from prop value. What about the value that is coming from URL? We are dependent on
onInputChange
to pass back the URL value as prop. What if this component is not used in the Form and we don't haveonInputChange
handler which updates the prop value. Will this component work?If this is the case, we are creating an incomplete error-prone component.
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.
Yes, it won't work if not used within form. But I suppose since it in an input component, it will always be used inside a form. Do you want it to be more independent, generic, futureproof, and have its own state?
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.
Currently it is not used without a
Form
and If it is used as a standalone component, it can always be wrapped around aForm
likeIncorporationStateBusiness
. Or the devs can modify the code as per their needs. For current uses and its intended purpose, do you think it still needs a local state?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.
Current StatePicker too doesnt have local state. Since we're just mimicking its functionality, do you still think we need to worry about local state?
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.
Ok, I am fine with its current state and it can be modified later if needed. But let's add explanatory comments to the logic above to explain what's going on in the effect and how this component is working.
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.
Sure, done.