This repository has been archived by the owner on Jun 24, 2022. It is now read-only.
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.
Create and implement a Stepper component. #2246
Create and implement a Stepper component. #2246
Changes from 9 commits
aee5ea5
f5e2dc4
8a44672
54952c4
3bfe1bb
98b26be
6d2c04e
9ea99b9
439f761
bd4e4a5
0817c89
b3025a6
4ad4781
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
bit overkill as there are only 3 steps but cool anyhow
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.
Just keeping in mind, this was setup as a re-usable component (outside
Claim
purposes).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.
why do you need an id if the index/order is deterministic? it's an array
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.
@W3stside My thinking was that this would be a more fool proof approach. Order of an
Array
in the context of this small Array of objectsconst
indeed is deterministic.In context of a general purpose
Stepper
component, my understanding is that in scenarios where you might have missing (object) values in the Array or externally imported data, one could argue setting explicit step numbers might be a defensive way of setting it up, given the order/index number might be affected in those scenarios. But correct me if I'm wrong.Happy to do it another way, but just to highlight.
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.
Feel free to review post merge. For now I reverted logic back to use
index
for key.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.
Then make id optional as a prop and use the index as a fallback
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 Michel, makes sense if you plan to feed this with dynamic data which doesn't have order guarantees.
My thinking was, it's just using a static deterministic array, and in practical terms it will always be the case. I don't see the stepper consuming a dynamic list from an untrusted REST API :P
Anyways, it's all good, i consider the comment a ubernit :)
Fine to leave it, fine to remove it, fine to make it optional
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 see, could also be an option. A bit messy potentially in the end where
index
andid
are thrown together and mixed up? I think I leave it as per your first suggestion and use onlyindex
for now. Feel free to modify post merge otherwise.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.
@anxolin Yeah, probably I'm over thinking it at this moment :)