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

Editor Design System #69

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

Conversation

coreh
Copy link

@coreh coreh commented May 8, 2023

@PROMETHIA-27
Copy link

I would note for the color section that special attention should be given to accounting for colorblind people; never use only color to signify meaning, always combine it with an additional identifier such as an icon.

@james7132 james7132 self-requested a review May 8, 2023 22:07
Copy link

@djeedai djeedai left a comment

Choose a reason for hiding this comment

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

This RFC is starting to go in all directions, having:

  • some elements about high level design, colors, the general organization of the Editor UI
  • one step lower level, the various widgets,
  • one more step lower level, some spec about widget implementation at the code level

This is clearly too broad for an RFC.


### Standard Widget Patterns

#### Bundle Structure
Copy link

Choose a reason for hiding this comment

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

This seems completely unrelated with the layout and color discussion from above.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, that's fair. This might make sense as an entirely separate RFC. (i.e. a "Standard Widget Bundles RFC")

I'm including this here while in draft mode because it's something I want to keep in mind when structuring the proof of concept I'm building, and also something I want to get early feedback/validation on, not necessarily it needs to stay before it becomes an actual non-draft RFC.


Standard widgets from `bevy_editor_ui` adhere to a standard bundle structure. For a standard widget named `MyWidget`:

| Component | Required? | Role | Access By Widget's Systems | Access By Widget's User | Reactive?
Copy link

Choose a reason for hiding this comment

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

This entire table is unreviewable until we have a clear idea (at least) about the UI architecture we want, which we don't. We're still exploring patterns of UI and ECS integration and I've yet to see something compelling on paper, let alone an actual RFC about it.

  • Data being owned by the widget excludes an entire class of designs (like Druid's, were widgets borrow and mutate data in place)
  • The write-only widget out excludes another entire class of design implementation (React's single source of truth were data is mutated in-place and diff-ed for changes)

Copy link
Author

Choose a reason for hiding this comment

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

This entire table is unreviewable until we have a clear idea (at least) about the UI architecture we want, which we don't. We're still exploring patterns of UI and ECS integration and I've yet to see something compelling on paper, let alone an actual RFC about it.

If unreviewable, I can definitely leave it out of the RFC once it's out of draft. But IMO, the Editor UI architecture can afford to be far more opinionated and comprehensive than the general UI architecture we expose in Bevy UI, since it will be consumed primarily internally from within the same repo; the churn of upgrading in case we change our minds is much smaller. (We can even specifically flag it as experimental, so that people don't build third party plugins for the editor initially expecting upgrading will be easy)

  • Data being owned by the widget excludes an entire class of designs (like Druid's, were widgets borrow and mutate data in place)

Hmm. Haven't looked into Druid yet, so I can't say I fully understand what a borrowing/mutation-first taken on a UI framework actually entails, but I don't see owning data as mutually exclusive with tapping into data from somewhere else via mutable or immutable references. The owned data could be a predicate that allows indirectly controlling borrowing and mutating data in place, like a search filter closure for a query, an Entity or maybe even a reflected path into some component.

For example: in my proof or concept, I'm using a unit struct plus a trait with an associated type to display arbitrary entities with a specific component as items in a tree view, but the tree view filter (for searching) will be owned by the component as local state.

  • The write-only widget out excludes another entire class of design implementation (React's single source of truth were data is mutated in-place and diff-ed for changes)

I got the idea for this specific pattern from React, as it's analogous to the forwardRef()/useImperativeHandle() mechanism. React has two "ways" of handling things like form inputs and other interactive elements: controlled and uncontrolled.

  • Controlled has the parent be the source of truth, and requires all changes to be first propagated from the child to the parent via events (e.g onChange). The parent event handler issues a local state update, and the parent "re-renders", then the child is reconciled and also "re-renders". This is the pattern you are likely referring to;
  • Uncontrolled has the child be the source of truth, so changes are applied internally as local state updates. The parent can then use the child's exposed imperative handle (obtained via a ref) to read the data only when needed. (e.g. when submitting a form)

Both preserve the single source of truth for each piece of information, the difference is just where it ends up being kept. The same codebase will often up using both patterns for different things, depending on whether one needs to react to all changes granularly or just read a snapshot after a specific action. (Controlled is very boilerplate-y, and has potential performance implications if the parent is too complex)

Note: React also exposes methods in this imperative handle, but I think we might be better served by something else like an event-based mechanism for this.


With default settings, the Bevy Editor UI takes the following high-level structure:

![Default UI Layout, showing: Menu and action bars at the top. Toolbar at the left. Status bar at the bottom. Several “tiles” in the center for project, scene, viewport and inspector.](assets/69-editor-design-system/default-layout.png)
Copy link

Choose a reason for hiding this comment

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

Is this a grease pencil of the various screen areas, or an actual mock-up with proper style/color? Is the latter, I find the color saturation way too high and distracting.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, this is just a schematic view to block out rough layout.


#### Command Palette

This is a transient, modal floating UI popup that allows globally searching and running actions, switching between open editors, opening new items, and altering properties and settings via keyboard.
Copy link

Choose a reason for hiding this comment

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

switching between open editors,

Seems out of place. A command palette is a list of commands (actions to perform on user data). Switching views is not an action, it's merely a focus change in most cases.

Copy link
Author

Choose a reason for hiding this comment

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

VS Code has a couple of character prefixes (>, @, :) to allow commands, switching between open files, or going to specific symbols or lines in a file, while still sharing the same text field with no namespace clashes, I think we could go for something similar.


### Visual Style

#### Color Palette
Copy link

Choose a reason for hiding this comment

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

This should be the default color palette as per accessibility discussion.

Copy link
Author

@coreh coreh May 16, 2023

Choose a reason for hiding this comment

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

Yeah, I will actually rename the entire parent section to default visual style, and add a disclaimer about it. I didn't anticipate how much theming would be something people would be interested/invested in.

From the Discord conversations theming and customization might actually be a very high-priority requirement, beyond just the accessibility concerns, and something we get a lot of community contributions, and which could be a lot of fun too. (e.g. 🎃 A halloween theme, or theme-jams)

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.

None yet

3 participants