palexdev / MaterialFX

A library of material components for JavaFX
GNU Lesser General Public License v3.0
1.22k stars 121 forks source link

Feature Enhancement Proposal: `CSSFragment` API Improvements #388

Open infinite-dev22 opened 1 month ago

infinite-dev22 commented 1 month ago

I’d like to propose some enhancements to the CSSFragment API, which could further streamline development workflows and align the API for convenience and clarity.

Proposed Enhancements for CSSFragment

After working with the current API, I believe the following improvements could streamline its functionality and enhance the developer experience:

  1. Support for Enum-based Selectors Enhancing the .addSelector() method to accept predefined, enum-like values representing standard CSS selectors would improve readability and reduce errors associated with raw string usage. This approach would be particularly beneficial when working with complex or multi-selector styles.

  2. Direct Numeric Input for Size-related Properties Allowing direct numeric input for properties such as padding, margin, width, angles, etc. would enhance the intuitiveness of the API. By accepting int or double values, developers could eliminate repetitive string conversions, leading to a more streamlined and error-resistant workflow for numeric style properties.

  3. Implicit Selector Closure in .build() Integrating .closeSelector() functionality within .build() would simplify the usage of CSSFragment by removing the need for explicit selector closure calls. This modification would yield a cleaner API that supports a more natural chaining style, particularly beneficial when applying multiple styles in sequence.

Minimal Reproducible Example (MRE)

To illustrate, here’s an example using the proposed improvements:

CSSFragment(sp).addSelector(".css-class-selector")
               .padding(20)
               .background("transparent")
               .build();

This design streamlines the current approach, which requires multiple calls as follows:

CSSFragment.Builder.build()
                   .addSelector(".css-class-selector")
                   .padding("20px")
                   .background("transparent")
                   .closeSelector()
                   .applyOn(sp);

Thank you for considering these enhancements.

palexdev commented 1 month ago

Thank you very much, will look into it asap

infinite-dev22 commented 4 weeks ago

Is it ok if I keep editing the enhancement request as I realise more suggestions.

palexdev commented 4 weeks ago

Is it ok if I keep editing the enhancement request as I realise more suggestions.

Absolutely! Feel free to keep updating it as new ideas come to mind.

palexdev commented 3 weeks ago

Hey @infinite-dev22, can you show me what you mean by Support for Enum-based Selectors?

I see this CSSFragment(sp).addSelector(".css-class-selector") but it's not very explicative. Do you mean something like this?

Node myNode...
CSSFragment.Builder.build()
    .addSelector(myNode)
    ...
palexdev commented 3 weeks ago

@infinite-dev22, major changes are now available, check CSSFragment on rewrite

Let me know if there's something else you want to see, or if the new changes still do not meet your expectations 😉

infinite-dev22 commented 3 weeks ago

Hey @infinite-dev22, can you show me what you mean by Support for Enum-based Selectors?

I see this CSSFragment(sp).addSelector(".css-class-selector") but it's not very explicative. Do you mean something like this?

Node myNode...
CSSFragment.Builder.build()
    .addSelector(myNode)
    ...

Something more like: CSSFragment(sp).addSelector(CSSSelector.SCROLL_PANE) // Especially for MaterialFX and JavaFX selectors supported in MaterialFX and VirtualizedFX .padding(20) .background("transparent") .build();

This might not be globally applicable, It's only that I use MaterialFX and VirtualizedFX alongside Plain Old JavaFX in my Projects.

infinite-dev22 commented 3 weeks ago

@infinite-dev22, major changes are now available, check CSSFragment on rewrite

Let me know if there's something else you want to see, or if the new changes still do not meet your expectations 😉

Most are covered, only the one of .applyOn() but that's ok

palexdev commented 3 weeks ago

Hey @infinite-dev22, can you show me what you mean by Support for Enum-based Selectors?

I see this CSSFragment(sp).addSelector(".css-class-selector") but it's not very explicative. Do you mean something like this?

Node myNode...
CSSFragment.Builder.build()
    .addSelector(myNode)
    ...

Something more like: CSSFragment(sp).addSelector(CSSSelector.SCROLL_PANE) // Especially for MaterialFX and JavaFX selectors supported in MaterialFX and VirtualizedFX .padding(20) .background("transparent") .build();

This might not be globally applicable, It's only that I use MaterialFX and VirtualizedFX alongside Plain Old JavaFX in my Projects.

Oh I see. The problem with such approach is that not only it's not globally applicable, but also a lot of work to implement and maintain. For every node in JavaFX, MaterialFX and any other library we would have to write an enumerator class. And if in the future one of those nodes changes its selectors, then the enumerator becomes invalid and would have to be updated. For these reasons I think the enum approach could be implemented locally on your project but not here

Most are covered, only the one of .applyOn() but that's ok

I could implement this possibility:

Node owner;
new CSSFragment.Builder(owner)
    .select(...)
    .style(...)
    .build() // Automatically apply on owner

Although it poses a few questions: wouldn't it be redundant, like is it really avoiding complexity/an improvement over applyOn? The owner node indicates only the node on which to apply the CSS or should it also call select(owner) upon creation?

infinite-dev22 commented 3 weeks ago

Initially I had proposed it as

CSSFragment(sp).addSelector(".css-class-selector")
    .padding(20)
    .background("transparent")
    .build();

Which can be update as per current as:

CSSFragment(sp).select(...)
    .style(...)
    .build();

With sp being what currently refer to as owner node. After looking at the code, your approach is much easier to implement without having to tamper with code in CSSFragment as the former proposed enhacement would required completely removing Builder inner class and pass on it's duties to CSSFragment.

Your approach:

Node owner;
new CSSFragment.Builder(owner)
    .select(...)
    .style(...)
    .build() // Automatically apply on owner

Now with the above code, it is really is redundant

infinite-dev22 commented 3 weeks ago

Also I had proposed Direct Numeric Input for Size-related Properties but then there are different sizing Units in JavaFX CSS. I thought creating extensions like 20.px, 1.5.em but this isn't possible in Java or JavaFX as ava doesn’t support custom literal suffixes.

palexdev commented 3 weeks ago

Initially I had proposed it as

CSSFragment(sp).addSelector(".css-class-selector")
    .padding(20)
    .background("transparent")
    .build();

Which can be update as per current as:

CSSFragment(sp).select(...)
    .style(...)
    .build();

With sp being what currently refer to as owner node. After looking at the code, your approach is much easier to implement without having to tamper with code in CSSFragment as the former proposed enhacement would required completely removing Builder inner class and pass on it's duties to CSSFragment.

Your approach:

Node owner;
new CSSFragment.Builder(owner)
    .select(...)
    .style(...)
    .build() // Automatically apply on owner

Now with the above code, it is really is redundant

Ah I see. I would prefer to keep the builder as a separation of concerns. CSSFragment represents a stylesheet, the Builder helps you assemble it.

palexdev commented 3 weeks ago

Also I had proposed Direct Numeric Input for Size-related Properties but then there are different sizing Units in JavaFX CSS. I thought creating extensions like 20.px, 1.5.em but this isn't possible in Java or JavaFX as ava doesn’t support custom literal suffixes.

Well, there are units technically, here

Which means I could do something like this: .padding(10, SizeUnits.PX) or .padding(1, 2, 3, 4, SizeUnits.PX) There are pros and cons to this though

infinite-dev22 commented 3 weeks ago

Which means I could do something like this: .padding(10, SizeUnits.PX) or .padding(1, 2, 3, 4, SizeUnits.PX)

This is much better than how I'd thought of handling it.

There are pros and cons to this though

What could the pros and cons be to this approach.

In most common cases, most used Unit is pixels(px) which I think is the JavaFX default hence I think having to cater if no SizeUnit is provided could fall back to the default.

palexdev commented 3 weeks ago

Which means I could do something like this: .padding(10, SizeUnits.PX) or .padding(1, 2, 3, 4, SizeUnits.PX)

This is much better than how I'd thought of handling it.

There are pros and cons to this though

What could the pros and cons be to this approach.

In most common cases, most used Unit is pixels(px) which I think is the JavaFX default hence I think having to cater if no SizeUnit is provided could fall back to the default.

One clear advantage is the ability to work with different units without relying on strings. However, the downside is that supporting mixed units, while possible, is extremely challenging to implement and maintain. For example, insets such as 1px 1em 1px 1pt. However, it's also true that such cases are extremely rare.

Edit: it would indeed be possible to do something like .padding(1, 2, 3, 4, PX, EM, PX, PT), but I feel like it would become too verbose. Like, I get that using Strings may be error-prone, but you have no constraints with them. While I agree with some of these enhancements, it feels like we're trying to cover every possible case to replace Strings completely, which is not ideal in my opinion

Edit 2: It's also important to determine for which properties this could be useful. Insets, font size, widths and heights, what more?