Skip to content
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

feat(components): Disclosure (fka Accordion) #5873

Merged
merged 8 commits into from
Jan 3, 2025

Conversation

cephalization
Copy link
Contributor

@cephalization cephalization commented Jan 2, 2025

Port from Accordion to React Aria Disclosure and DisclosureGroup

New.Accordion.mov

Resolves #5855

Port from Accordion to React Aria Disclosure and DisclosureGroup
Comment on lines 3 to 13
export const disclosureGroupCss = css`
& > * {
width: 100%;
.react-aria-Heading {
width: 100%;
.react-aria-Button[slot="trigger"] {
width: 100%;
}
}
}
`;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking of just putting this on the disclosure itself and not having any styles for the group.

Right now you have to wrap a single Disclosure in a group if you want it to fill its container, otherwise its only as long as its title.

Either that, or I add a width prop to Disclosure

>
<PlaygroundTemplate
<DisclosureGroup defaultExpandedKeys={["prompts"]}>
<Disclosure id="prompts">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This originally had a size of "L". Stashing and unstashing this, I can't really tell a difference in size

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you may be right that there's only one size

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out L was valuable. It made the accordions on this page all the same height, which matters when one accordion has nested controls

@cephalization cephalization marked this pull request as ready for review January 3, 2025 02:16
@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Jan 3, 2025
<ModelEmbeddingsTable model={data} />
</DisclosurePanel>
</Disclosure>
<Disclosure id="dimensions">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a missing border here at the bottom of the table.
Screenshot 2025-01-02 at 7 40 09 PM

You can run this via pnpm run dev:embeddings

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran through all of the embedding usages before pushing. I honestly can't tell which border is missing from this screenshot 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect there to be a visual separation between the table pagination and the row that says "Dimensions"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Disclosure group will now add a border between disclosures, as long as it is expanded and not the last item

Copy link
Contributor

@mikeldking mikeldking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one - some minor style nits. LMK if you want another set of eyes

Comment on lines +264 to +269
<StopPropagation>
<Flex direction="row" gap="size-100" alignItems="center">
<TemplateLanguageRadioGroup />
<AddPromptButton />
</Flex>
</StopPropagation>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bit of visual regression here where I think the "actions" should still be right aligned

Also the addition of the nested values causes the trigger hight to grow. Can we use a different sizing strategy so that these triggers stay the same height? I know it looks odd padding wise but it might be the lesser of two evils right now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

I added a "L" variant that makes all of these disclosures the same height now

app/src/pages/playground/Playground.tsx Outdated Show resolved Hide resolved
<ModelEmbeddingsTable model={data} />
</DisclosurePanel>
</Disclosure>
<Disclosure id="dimensions">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect there to be a visual separation between the table pagination and the row that says "Dimensions"

app/src/pages/trace/SpanFeedback.tsx Show resolved Hide resolved
@mikeldking
Copy link
Contributor

I think the padding on the text used to match the tab indentation.
Screenshot 2025-01-02 at 7 54 30 PM

see https://phoenix-demo.arize.com/projects/UHJvamVjdDoy/traces/80e4a4d13fe96c5c55393004345fbba0?selectedSpanNodeId=U3BhbjoxNDk4NQ==

@cephalization cephalization changed the title feat(components): Disclosure feat(components): Disclosure (fka Accordion) Jan 3, 2025
Copy link
Contributor

@mikeldking mikeldking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double approved

@cephalization cephalization merged commit ea8a7c7 into main Jan 3, 2025
14 checks passed
@cephalization cephalization deleted the cephalization/5855-phoenix-components-accordion branch January 3, 2025 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[components] Accordion
2 participants