fix: ResponseStatus QA - #10384
Conversation
|
Build successful! 🎉 |
|
Build successful! 🎉 |
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
Not sure what happened to the spacing between items? was that intentional?
Should all the content of the top level response status be only 2px in from the outside, it either doesn't look like enough to visually separate, or just ever so slightly off if it's supposed to align. It looks like the values match Disclosure, so I'm betting it's an issue with the padding on the ol, maybe reduce it from 4 to 2?
| } | ||
|
|
||
| const detailTriggerStyles = style({ | ||
| display: 'block', |
There was a problem hiding this comment.
Is this correct? It looks like the child has CenterBaseline for an icon, so this should be display flex already.
Then I think we can remove the marginStart from detailTriggerChevronStyles
There was a problem hiding this comment.
The display: block is to handle the disclosure chevron aligning with the title when wrapping to a second line - which was an ask during the initial demo I did with the RSP team
without:
with/current:
There was a problem hiding this comment.
I think we can remove the marginStart from detailTriggerChevronStyles
The marginStart is to override the value set on buttonStyles that is shared between the parent and child disclosures:
paddingX: 'calc(self(minHeight) * 3/8 - 1px)',
The buttonStyles definition was copied from Disclosure for the initial ReponseStatus component and equates to 11px. Since there will be some spacing iteration when we get final design specs, I opted to take the path that prioritized keeping our new ExectutionTraceItem spacing to the system spacing values rather than a bunch of +1/-1 odd numbers to get the title+panel content to line up while giving the title's focus ring breathing room
Update:
mix up -- my comment above is re: detailTriggerStyles instead of detailTriggerChevronStyles
but does tie into answering the spacing question in this other comment you left:
Should all the content of the top level response status be only 2px in from the outside, it either doesn't look like enough to visually separate, or just ever so slightly off if it's supposed to align. It looks like the values match Disclosure, so I'm betting it's an issue with the padding on the ol, maybe reduce it from 4 to 2?
|
|
||
| const executionTraceDisclosureContainerStyles = style({ | ||
| paddingBottom: 12, | ||
| marginTop: -4 |
There was a problem hiding this comment.
I think we could get rid of this negative margin if we changed the item to display as a grid
template areas
[['icon', 'button'],
['line', 'content']]
then we could center baseline the icon vertically in it's cell, which would be the same height as the text, even if the text were to wrap (don't know if that's a thing)
the line would just have stretch set
I think this would also help with eventual changes to font size or tshirt size
There was a problem hiding this comment.
For the ExecutionTraceItems that are disclosures, the button height is the 32px min tap target which is 8px larger than the text-only item titles. The -4px is to visually center the button with the icon without increasing the space between the divider and icon because most of the design explorations have nestled the divider and icon together as close as possible.
Then eventual move may be moving to a grid - but I feel Design needs to first work out their exploration of nested disclosures being included in the component (as this was an engineering-driven spike to help them understand the use case of chats having more complex content to report), which will hopefully take the tap target / divider / icon size relationship into consideration.
In a pass at switching to a grid, while it does offer center alignment without the -4px, we persist the problem that the button height drives the row height and then impacts the spacing of the divider+icon compared to the text-only items:
I appreciate your review vigilance and the attention to detail 🥷 -- this is just a Draft; I opened this PR to get a beta Storybook build to test on mobile Safari to see if the I'll be amending the commits before swapping this to |
eef7969 to
412bb89
Compare
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| '--execution-trace-item-padding-bottom-disclosure': { | ||
| type: 'paddingBottom', | ||
| value: { | ||
| default: 12, | ||
| ':last-child': 0 | ||
| } | ||
| }, | ||
| '--execution-trace-item-padding-bottom-no-disclosure': { | ||
| type: 'paddingBottom', | ||
| value: { | ||
| default: 16, | ||
| ':last-child': 0 |
There was a problem hiding this comment.
Some screenshots to show what this block accomplishes - both disclosures and non-disclosure items the same height with the different 12/16 padding (before any content might wrap and the last item not having padding:
…oesn't impact the height transition
…disclosure and non-disclosure items are the same min-height
412bb89 to
fe4515c
Compare
|
Build successful! 🎉 |
Agent Skills ChangesModified (2)
InstallReact Spectrum S2: React Aria: |
5 participants
✨ Changes:
li& also ensure that dislosure and non-disclosure items are the same height via padding size📝 Test Instructions:
ExecutionTraceItemsResponseStatusand childExecutionTraceItems and see focus ring hug content