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
UI: add subkey request to kv v2 adapter #27804
UI: add subkey request to kv v2 adapter #27804
Changes from 13 commits
02950f4
1254e72
4b1437b
40a150b
6b5e8e8
b8fa780
94f1e9b
706d1e8
06059d4
5e0efa5
49a60be
a1b165f
6e56f36
ad53d79
cf02311
8b27c52
670dd13
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.
Any reason you didn't export a
kvSubkeyPath
method instead of using the base helper? Then the version and query params logic could be in there as well, and easy to unit testThere 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.
Since this endpoint isn't implemented yet, it was easier for my brain to fiddle with and update the logic in one place. I can move this into a
kvSubkeyPath
helper. The adapter unit test felt sufficient for testing, but I can add another test for the util.Also the default args for this endpoint work a little differently since we always send a
depth
param but not always a version param, so I can't use the version logic out of the box frombuildKvPath
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.
since this is a new class, I opted to use the updated
-12
number naming which is a pattern we had discussed moving towards (and away froms
,m
,l
) happy to revert and make this-s
if folks preferThere 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.
Can you just add a comment here to that affect? That this is the pattern we want to move toward, away from s/m/l so that future code-browsers will not be confused by the different types of measurements
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, good idea! Will add
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.
Although this affects overview cards everywhere, it's a very small change and I'd rather the spacing be consistent across the board than have different padding situations.
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.
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 didn't want to update
@subText
for allOverviewCards
, I felt like it made sense to keep both potential use cases. 1) just text subtext and 2) more stylized content, such as inline linksThere 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.
Are
@subtext
and<:subtext>
mutually exclusive? Right now passing both with yield bothThere 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 intentionally made them not mutually exclusive as I didn't see a reason for them to be. Someone could add subtext in the default styling and then add a second stylized block. What do you think?
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.
Makes sense to me! I think the most confusing part is that they are named the same thing, so it felt like maybe they should be mutually exclusive. But, naming is hard 🤷
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.