refactor: take one product's concepts back out of the package - #6
Merged
Conversation
Re-read against a single question: is this a rendering component, or does it need to know something only a product knows. Four answers came back wrong, and all four had arrived by lifting product files rather than designing an interface. The sibling AI package, whose components were written against view models from the start, has none of them. `DataTable` wrote to `localStorage` itself. Where column preferences live, under which key, per user or per workspace, or whether they persist at all, is a decision only a consumer can make; a component that answers it cannot be reused by one that answers differently, and the table stopped working anywhere `localStorage` is absent. It now takes `columnState` and reports changes. `ErrorState` took an error category, one of network, configuration, model, permission, generic. "model" is one product's vocabulary, and the five names resolved to three colours anyway. It now takes a tone and an optional icon. `EmptyState` shipped ten drawings named after one product's screens: chat, models, creations, benchmark, logs, statistics, schedule. No other consumer has a "creations" screen to draw for. `illustration` is a node now, and the drawings go back to the product. This reverses 0.1.0-alpha.3, which unblocked that product's migration by widening the wrong side of the boundary. `Tabs` carried a four-name guide-tag system. That one had already announced itself: the badge variant lived here while the label text had to be passed in from the consumer's locale bundle, so a single concept sat in two repositories. A consumer renders its badge through the `labelExtra` slot that already existed. All four are breaking. They land while there is exactly one consumer and no stable release, which is the cheapest this will ever be.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
Four places where one product's concepts had come across with the code. Found by re-reading the package against a single question: is this a rendering component, or does it need to know something only a product knows?
DataTableErrorStateEmptyStateTabsAll four arrived the same way: by lifting product files rather than designing an interface. The sibling
@lablup/ui-ai, whose components were written against view models (AiToolCallView,AiRenderAdapters) from the start, has none of them. That difference is the lesson worth keeping.The four
DataTablewrote tolocalStorageitself. Where column preferences live, under which key, per user or per workspace, or whether they persist at all, is a decision only a consumer can make. A component that answers it cannot be reused by a consumer that answers differently, and it also stopped the table working anywherelocalStorageis absent.persistKeyis replaced bycolumnState+onColumnStateChange. Two new tests: the component touches no storage, and it does not report back the state it was handed, so a caller that persists on change and feeds the result in cannot loop.ErrorStatetook an error category."network" | "configuration" | "model" | "permission" | "generic". Beyond"model"being one product's vocabulary, the five names resolved to three colours: network and permission were the same amber, model and generic the same red. It now takestone: "danger" | "warning" | "accent"and an optionalicon.EmptyStateshipped ten drawings named after one product's screens. No other consumer has a "creations" screen to draw for.illustrationis aReactNodenow and the drawings go back to the product that owns those screens. This reverses the export added in0.1.0-alpha.3, which unblocked that product's migration by widening the wrong side of the boundary. Doing it that way was the expedient move at the time; this is the correct one.Tabscarried a guide-tag system. That one had already announced itself:TAG_CONFIGkept the badge variant here while the label text had to be passed in throughtagLabelsfrom the consumer's locale bundle. A single concept in two repositories is what a wrong boundary looks like from the inside.tag,required,TabTagType,TabTagLabels,TAG_CONFIGandtagLabelsare gone; a consumer renders its own badge through theTabItem.labelExtraslot that already existed, and the.tabs__tag-badgeclass stays so it still looks right.Breaking
All four. They land while there is exactly one consumer and no stable release, which is the cheapest this will ever be. Migration table is in the changelog.
Verification
pnpm run verifygreen, including the external install fixture. Packed files 140 to 134.