Skip to content

Conversation

@severo
Copy link
Contributor

@severo severo commented Apr 22, 2025

Can be used for iceberg versions, or git branches, for example.

See hyparam/space#3

Screenshot From 2025-04-22 23-40-41

Can be used for iceberg versions, or for git branches, for example.

See hyparam/space#3
@severo severo requested a review from platypii April 22, 2025 21:21
@severo severo requested review from bleakley and Copilot April 22, 2025 21:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces versioning support to the Breadcrumb component by adding new types and UI elements to display version or branch links.

  • Added TypeScript interfaces (Version, VersionsData) and integrated these into the Source type.
  • Updated configuration (useConfig) to include a versions property for styling.
  • Implemented a Versions component within Breadcrumb, with corresponding tests and a Storybook story demonstrating its behavior.

Reviewed Changes

Copilot reviewed 5 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/lib/sources/types.ts Added interfaces for version data and updated BaseSource to optionally include versions.
src/hooks/useConfig.ts Added a configuration key for versions to support custom styling for the new component.
src/components/Breadcrumb/Breadcrumb.tsx Introduced a Versions component that utilizes a Dropdown for rendering version links.
src/components/Breadcrumb/Breadcrumb.test.tsx Implemented tests to verify that version links are rendered correctly when supplied.
src/components/Breadcrumb/Breadcrumb.stories.tsx Added a Storybook story to showcase the Versions component integration.
Files not reviewed (2)
  • .storybook/global.css: Language not supported
  • src/components/Breadcrumb/Breadcrumb.module.css: Language not supported

Copy link
Contributor

@platypii platypii left a comment

Choose a reason for hiding this comment

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

nice abstraction

@severo
Copy link
Contributor Author

severo commented Apr 23, 2025

I'm not 100% happy with it because

  1. it's very specific (we might prefer passing children in some way) - I think it's OK
  2. it prevents having both iceberg versions and branches for example. - we can add Branches along Versions later if needed

But let's merge to have a branch selector in the space, and we can revisit later if needed.

@severo severo merged commit ccb1940 into master Apr 23, 2025
4 checks passed
@severo severo deleted the add-versions-to-source branch April 23, 2025 08:10
@severo
Copy link
Contributor Author

severo commented Apr 23, 2025

Hmmm I forgot that the Source is parsed synchronously in the app, while the list of branches or versions is generally fetched asynchronously. I'll work on a follow-up PR that fixes this (and maybe the other issues mentioned above)

severo added a commit that referenced this pull request Apr 23, 2025
@severo
Copy link
Contributor Author

severo commented Apr 23, 2025

See #226

severo added a commit that referenced this pull request Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants