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

[API review] Improve Jewel public API #84

Closed
rock3r opened this issue Jul 10, 2023 · 8 comments
Closed

[API review] Improve Jewel public API #84

rock3r opened this issue Jul 10, 2023 · 8 comments
Assignees
Labels
api Changes related to the public API release-blocker Issues that block a (stable) release of the library

Comments

@rock3r
Copy link
Collaborator

rock3r commented Jul 10, 2023

As follow-up to #83, we still have several improvements to make to the API, before proceeding to getting a proper API review session. After an initial discussion of things we can improve with members of the Jetpack Compose API council, these are the first few things we want to change:

1. Switch "compound" component overrides to slot APIs

We have some compound components, e.g., CheckboxRow and similar, that have overloads that take a string to provide a "complete" component. We should rather prefer to use slot-style APIs, since that is the usual pattern for Compose and it provides more and easier customization options. The effort for users to, for example, use a Text to fill in the slot is minimal, too.

We should also make sure that we avoid as much as possible using private APIs to implement our components, as this hinders reusability of building blocks from users, forcing them to copy-paste things even when not necessary.

2. Ensure component naming consistency

We should make sure that all our barebones, no-decoration "base" components follow the naming conventions set in the rest of Component, by being prefixed by the Basic word. For example, see TextField vs BasicTextField in Material and Foundation.

We should also rename OutlinedButton to just Button, since those are the buttons people think of by default. Users will not be familiar with names from the specs in Figma.

3. Get rid of styles

We should remove style parameters (which I introduced in 0.2.0, replacing the existing Defaults naming and conventions). Instead, we should either inline the values as default values for component parameters, if there are few of a type, or provide objects containing related defaults if there are several (or they depend for example on light vs dark). We cannot copy the Material approach entirely since we are effectively implementing a single theme that needs to support a minimum of 5 "feels"*.

The base/standalone feels are:

. Int UI Darcula
Light Int UI Light IntelliJ Light
Dark Int UI Dark Darcula

And to those we should add the Swing LaF Bridge "feel"; and eventually the High Visibility "feel", too (in the IDE, it'll come via the LaF Bridge theme, but we will need a separate one for standalone).


*Given that in Compose theme has a very specific meaning, that is different from what we use it for in Jewel, we should remodulate names accordingly. We need some new terminology since we face new requirements that Material does not. I propose:

  • Theme -> Jewel
  • Feel -> any supported combination from 3. — e.g., "Darcula", "Int UI Light", "Swing Bridge"
  • Style -> we should not use this term since it is confusing
@rock3r rock3r added api Changes related to the public API release-blocker Issues that block a (stable) release of the library labels Jul 10, 2023
@rock3r rock3r added this to the Build 0.4.0 (milestone 4) milestone Jul 10, 2023
@c5inco
Copy link
Collaborator

c5inco commented Jul 10, 2023

To the rename proposal, I think Jewel can be the base theme, and Darcula, Int UI Light can be application specific themes. I think Feel might be more confusing for those not super familiar with the IntelliJ Platform. Also I think Feel has an outdated meaning, since now the IntelliJ themes really only change look (putting aside the deltas between old and new UI).

@rock3r
Copy link
Collaborator Author

rock3r commented Jul 11, 2023

@c5inco you're right, I don't particularly like the "feel" term but couldn't find a better word. What we need to convey is that the theme is Jewel, which is a platform of sorts, and the various Int UI/Darcula/Bridge/Light/Dark combos are expressions of that one theme. They are not separate themes (nor could they, since that I learn entails having different sets of components)

@francisconoriega
Copy link

Hi first time posting, long time lurking :P

What about Jewel-Base, and then you'd have Jewel-Darcula, Jewel-Light, etc. It is a bit more verbose but it pretty explicit on the fact that this is the jewel version of Darcula/Light, and that they are all implemented on top of the base Jewel theme.

@rock3r
Copy link
Collaborator Author

rock3r commented Jul 12, 2023

Hi @francisconoriega, thanks for the input :) I wanted to avoid to put the Jewel name everywhere since it's an implementation detail in a certain sense, and ideally we'd want people not to need to know what it is; we haven't even named the base theme JewelTheme but, rather, IntelliJTheme for that reason.

@matvei-g
Copy link
Collaborator

FYI the guidelines to help with this have been published at https://github.com/androidx/androidx/blob/androidx-main/compose/docs/compose-component-api-guidelines.md

@rock3r
Copy link
Collaborator Author

rock3r commented Jul 24, 2023

Great! Thanks Matvei :)

@rock3r
Copy link
Collaborator Author

rock3r commented Sep 21, 2023

Related: #83, #99, and #120

@rock3r
Copy link
Collaborator Author

rock3r commented Oct 30, 2023

Work on this is ongoing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Changes related to the public API release-blocker Issues that block a (stable) release of the library
Projects
None yet
Development

No branches or pull requests

4 participants