added tabs to side bar, including hierarchy #120
Conversation
✅ Deploy Preview for storyteller-studio ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for storyteller-live-demo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Overall this looks good and works as expected! Feel free to take a look at the comments below and make any updates you feel comfortable with. You can ping me if you'd like me to take another look at the changes, but otherwise feel free to merge when you think it's ready to land.
There was a problem hiding this comment.
A few things here:
-
How would you feel about styling the scrollbar to be less invasive? It's pretty ugly on Windows. You can copy the scrollbar styles from
studio-web/src/lib/scene-hierarchy.element.scssif you don't have any objections. (And we'll extract this type of thing to Sass mixins soon so there's not so much copypasta going on with styles) -
Is it expected that most of the options don't have images currently? Are we waiting for a frontend update so those get populated?
-
Did you talk to @bflatastic about hiding the labels by default? I'm not sure if I love it, tbh. The missing images obviously make it worse, but even when the images are there I worry that it just makes it unnecessarily difficult to understand what the options actually represent.
If we do keep the hidden labels, we should probably give the same treatment to the gradient overlays, since they make the images a little harder to "read" and don't serve much purpose except making the labels more legible.
EDIT: Just got off the call with you re: this feature being moved into its own page. 😆 Feel free to ignore anything in this comment that won't be relevant for the new design.
There was a problem hiding this comment.
I don't even think we're going to keep that input for styles. We have a lot of styles and it doesn't seem to accommodate them well.
| </span> | ||
| </div>` | ||
| )} | ||
| <label>Select base style</label> |
There was a problem hiding this comment.
If we're making this a fully self-contained one-off instead of a generic/reusable form control, we should probably revert the class to extends LitElement and remove the override role property and override update method.
There was a problem hiding this comment.
I don't even know what override does, so we can get rid of it
There was a problem hiding this comment.
I don't even know what override does, so we can get rid of it
override is a TypeScript-specific keyword, indicating that you're overriding a property/method that exists in a parent class. I.e.:
class Dog {
bark() {
console.log("Woof")
}
}
class FrenchBulldog extends Dog {
override bark() {
console.log("Le woof")
}
}On a related note, if FrenchBulldog called super.bark() before its console log, then it would print "Woof" and then "Le woof" to the console, but the way it's written in that snippet it would only print "Le woof". That's why we always call the super method in our LitElement overrides, because they're usually doing important things that will cause the component to break if we forget it.
|
|
||
| protected override render = () => html` | ||
| <ul class="studio-tabs"> | ||
| <div class="studio-tabs"> |
There was a problem hiding this comment.
This is a tough thing to get used to coming from React, but a LitElement is an HTML element, so you don't actually need extra wrappers like this; they tend to just get in the way and make the CSS more complicated. (You also don't need to wrap everything in a Fragment or anything like that — you can just output multiple nodes from the root level of a Lit template.)
If you remove the div.studio-tabs, this component will render in the DOM like this:
<sts-tabs>
<div class="studio-tabs-tab">Label 1</div>
<div class="studio-tabs-tab">Label 2</div>
...
</sts-tabs>And you can style the <sts-tabs> element in your stylesheet with the :host pseudo-class selector — basically just moving all the style rules that are currently under .studio-tabs { ... } into :host { ... }.
There was a problem hiding this comment.
yeah I figured that out while working on the overall inspector/sidebar, it's just a remnant
| ${ this.tabs.map(({ label, value: itemValue },i) => | ||
| html`<li | ||
| class=${ itemValue === this.value ? "selected-tab" : "" } | ||
| html`<div | ||
| class=${ | ||
| `studio-tabs-tab${ itemValue === this.value ? " selected-tab" : "" }` | ||
| } | ||
| @click=${ ({ target }: any) => { | ||
| console.log("💎"); | ||
| this.onChange({ target: { name: this.name, value: itemValue } }); | ||
| } } | ||
| >${ label }</li>` | ||
| >${ label }</div>` | ||
| )} |
There was a problem hiding this comment.
This is kind of an advanced pattern to implement, so don't sweat it for now if it's confusing, but for generic components like this I would prefer a slot-based API instead of taking an array of JS objects as a property.
The <slot> element is basically the web-component equivalent to React's children prop. It's a little more idiomatic than using JS arrays to render HTML elements, and makes the component more flexible to reuse since you can more easily customize the styling and content of the individual tabs.
A hypothetical slot-based approach would look something like this:
@customElement("sts-tabs")
export class TabsElement extends LitElement {
@property() activeId?: string;
#slotRef = createRef<HTMLSlotElement>();
@on("click", { captures: true })
onClick(event: PointerEvent): void {
// When a child `<sts-tab>` is clicked, we'll emit an event to tell
// our parent that the clicked tab should become the active tab
if (event.target instanceof TabElement)
this.dispatchEvent(new CustomEvent("activeId-change", {
detail: event.target.id,
}));
}
// We'll want to manage the `selected` state of our child tabs whenever...
// 1) ...this component is added to the DOM
// (like a `useEffect` with no dependencies)
override connectedCallback(): void {
super.connectedCallback();
this.updateSelectedTab();
}
// 2) ...this component's `activeId` property changes
protected override update(changes: PropertyValues<this>): void {
if (changes.has("activeId")
this.updateSelectedTab();
super.update(changes);
}
// 3) ...if/when the slot's children change
// (e.g., if our parent is rendering some tabs conditionally)
onSlotChange(): void {
this.updateSelectedTab();
}
// And here's where we manage that state
updateSelectedTab(): void {
// Find the tab in our <slot> whose ID matches the `activeId` and
// mark it selected, while marking all the other tabs as unselected
for (let element of this.#slotRef.value?.assignedElements() ?? [])
if (element instanceof TabElement)
element.selected = (element.id === this.activeId);
}
protected override render = () => html`
<slot
${ref(this.#slotRef)}
@slotchange=${this.onSlotChange}
></slot>
`;
}
@customElement("sts-tab")
export class TabElement extends LitElement {
@property({ type: Boolean })
selected = false;
protected override update(changes: PropertyValues<this>): void {
// Set a "selected" class on this tab when the "selected" property is true
if (changes.has("selected")
this.classList.toggle("selected", this.selected);
super.update(changes);
}
protected override render = () => html`
<slot></slot>
`;
}It's a little more complicated to build the Tabs component this way, but as a result the component becomes way more flexible to work with:
type TabId
= "lorem"
| "ipsum"
| "dolor"
@customElement("my-app")
export class AppElement extends LitElement {
@state() activeTabId: TabId = "lorem";
onActiveTabChange({ detail: tabId }: CustomEvent<string>) {
// Could instead just use `bind(this, "activeTabId)` in `render`,
// but wanted to make it more clear what's happening in the example
this.activeTabId = tabId;
}
protected override render = () => html`
<sts-tabs
.activeId=${this.activeTabId}
@activeId-change=${this.onActiveTabChange}
>
<sts-tab id="lorem">
Lorem
</sts-tab>
<sts-tab id="ipsum">
<!-- We can put custom markup into the tabs now -->
<sts-icon icon="cube"></sts-icon>
Ipsum
</sts-tab>
<!-- We can customize the styling of individual tabs
since we can add our own classes -->
<sts-tab id="dolor" class="special-tab">
Dolor
</sts-tab>
</sts-tabs>
<div class="tab-content">
<!-- The 'match' function in "@storyteller/utility"
is basically just a fancy 'switch' -->
${match (this.activeTabId, {
"lorem": () => html`
The "Lorem" tab is active
`,
"ipsum": () => html`
The "Ipsum" tab is active
`,
"dolor": () => html`
The "Dolor" tab is active
`,
})};
</div>
`;
}Again, don't sweat it for now if this is too confusing or complicated, just wanted to kind of introduce you to the idea and try and explain how it works. I use this kind of pattern a lot, so you'll probably see it come up in some other places, but this particular example was a good opportunity for an introduction since there aren't too many moving parts or other things going on at the same time.
Let me know if you have any questions or if anything is unclear!
There was a problem hiding this comment.
this seems like a cumbersome pattern to use regularly but I have no preference
There was a problem hiding this comment.
It's definitely cumbersome to write, but my philosophy about complexity is that it's okay for stuff that's highly reusable to be complicated if it makes it easier to use, because you're probably going to be using it a lot more than you'll be updating the underlying code (especially if it's flexible enough to fit a wide variety of use cases without needing to be changed). I'd rather write something complicated/annoying once and then be able to rely on it forever than have to constantly revisit it and add a bunch of extra properties because now we need to support icons, or now we need another version with slightly different styling, etc. etc.
| } | ||
| @click=${ ({ target }: any) => { | ||
| console.log("💎"); | ||
| this.onChange({ target: { name: this.name, value: itemValue } }); |
There was a problem hiding this comment.
Would you mind converting this onChange callback prop to a custom-event dispatch, similar to how we did it in the BubbleSelect component?
2 participants
changes side bar from grid to accommodate a tab view. Style tab not functional