Skip to content

feat: add SCSS theme with css variables support #367

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

Merged
merged 10 commits into from
Jun 3, 2024

Conversation

Harpush
Copy link
Collaborator

@Harpush Harpush commented Nov 28, 2023

Adds support for SCSS theme with all options as CSS variables too.

Closes #331
Closes #248

@@ -118,7 +118,7 @@ export class SplitAreaDirective implements OnInit, OnDestroy {
})

const iframeFixDiv = this.renderer.createElement('div')
this.renderer.addClass(iframeFixDiv, 'iframe-fix')
this.renderer.addClass(iframeFixDiv, 'as-iframe-fix')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This a breaking change but seems more correct... @beeman worth it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about adding both while we are on v17.x.x with a deprecation warning, then removing it for v18?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is a bit of a gray area... We never explicitly said this is public api. I think if we are going the deprecation route - just stay with it as it is now because it is not explicitly public api so no real need for the as prefix

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say, do the change. We can put a small piece of description in the release notes that we renamed a private part of the API. I would not count that as a breaking change. more a "breakingish?" one? (aka a note in the release notes)

@Harpush Harpush requested a review from beeman November 28, 2023 00:09
@Jefiozie Jefiozie requested review from SanderElias and Jefiozie and removed request for beeman January 22, 2024 11:52
@SanderElias
Copy link
Contributor

Ok, I reviewed this.
If we are going to make a breaking change, I would say, own it, move away from SCSS, and move to CSS.
There is no need for sass anymore.
We can expose the public CSS API fully as custom CSS props(vars), and put all it into its own layer.
Write a small doc on how to update the use of it.

@Harpush
Copy link
Collaborator Author

Harpush commented Jan 23, 2024

Ok, I reviewed this. If we are going to make a breaking change, I would say, own it, move away from SCSS, and move to CSS. There is no need for sass anymore. We can expose the public CSS API fully as custom CSS props(vars), and put all it into its own layer. Write a small doc on how to update the use of it.

The PR had three main goals:

  1. Allow material like scss theme
  2. Allow css variables customization
  3. Get rid of all the ng-deep

Using only css variables means the scss goes back to the component. In order to answer goal 3 I need to make the area a component instead of a directive (which I think is a good idea anyway).

So my take on what's needed here:

  1. Make templates an html file instead of inline (not mandatory - but I really don't like having it all in one file)
  2. Change split area to a component and move css around to avoid ng deep
  3. Introduce css variables used inside both components

What's your take on it? Might even eliminate the breaking changes here aside from the iframe fix naming.

@Jefiozie
Copy link
Member

@all-contributors please add @Harpush for code

Copy link
Contributor

@Jefiozie

I've put up a pull request to add @Harpush! 🎉

@Jefiozie
Copy link
Member

@Harpush could you update your branch with the latest changes.

@SanderElias
Copy link
Contributor

@Harpush
If we are going to get rid of scss we don't need a separate template file.
I have some time tomorrow to dig into the actual code, after that, I will have a more nuanced opinion.

for now, I would like to see:

  1. a global 'css' "theme" file that holds the "public API" with the CSS vars people can modify
  2. no more ng-deep. css-vars can handle that.

I might update this after I spend some time in the code.

@Harpush
Copy link
Collaborator Author

Harpush commented Jan 25, 2024

@Harpush If we are going to get rid of scss we don't need a separate template file. I have some time tomorrow to dig into the actual code, after that, I will have a more nuanced opinion.

for now, I would like to see:

  1. a global 'css' "theme" file that holds the "public API" with the CSS vars people can modify
  2. no more ng-deep. css-vars can handle that.

I might update this after I spend some time in the code.

What does css theme file means? A file with :root with defaults?
And avoiding ng deep requires a component instead of a directive as far as I am aware.
Concerning html as a separate file - just wanted it for convenience especially with a scss file too.

@Jefiozie
Copy link
Member

@SanderElias could you elaborate your ideas here?

@Jefiozie
Copy link
Member

Jefiozie commented Mar 9, 2024

@Harpush are you waiting on feedback off @SanderElias ?

@Harpush
Copy link
Collaborator Author

Harpush commented Mar 9, 2024

@Harpush are you waiting on feedback off @SanderElias ?

Yes I am. Not sure I entirely understand what he asked for

@Harpush
Copy link
Collaborator Author

Harpush commented Apr 27, 2024

So what's the status of this PR @SanderElias @Jefiozie? Been stuck for some time now

@SanderElias
Copy link
Contributor

@Harpush I'm Sorry. I was otherwise engaged, and this slipped my attention for way too long.
Let me quickly catch up.

@SanderElias
Copy link
Contributor

SanderElias commented Apr 27, 2024

You are right about the component, but that would be a big, breaking change.
It would require a refactor of everyone using our library.
@Harpush Do you think there is a way we can keep the directive and mark it deprecated while adding it as a component?

About the inline templates to external files, I have a slight preference for keeping them inline. That also tends to keep the template parts smaller, which is better for a library. But if working with external templates works better for you, I would accept a PR that does that.

What does css theme file means? A file with :root with defaults?
Exactly this. Exposing the default values we need.
Or we expose something like this:
image

The :where gives it a specificity of zero, which means it can work as a 'fallback', allowing the end-user to overwrite it with anything they need, wherever they need it. The can add the values they want to set to :root or put different values on every different place they want to deviate. Or, do nothing, and use the defaults.

I see us moving to CSS so we don't oppose SCSS to users. Our Theme should be easily usable for all users, including those who are not using SCSS.

@Harpush
Copy link
Collaborator Author

Harpush commented Apr 27, 2024

@SanderElias
Concerning css theme - I understand now what you meant. Where would it be though? In the component scss file? Seems a bit weird but maybe I am wrong here as I have never created such css theme before.

Concerning directive vs component - as we are not using standalone components/directives I believe 99% of the users will not observe a breaking change. If by any chance someone uses the attribute selector there will be a breaking change here - though I am not sure how many actually use it.

The only 100% non breaking option seems like keeping the ng deep while still moving to css variables. And a later breaking PR to migrate the directive to a component and get rid of the ng deep. Another option is doing it all in one breaking PR for v18.

Do keep note that ditching scss entirely is something even angular material haven't done yet.

@SanderElias
Copy link
Contributor

@Harpush, we can export a centralized 'as-split-styles' file.
As we are using Angular, I would actually prefer to use ViewEncapsulation.none and put it next to the components. (But I'm not 100% sure this will work, so a quick experiment is needed!)
But if we do it like that, it is easy to maintain. However, that would require documentation on the CSS-custom-properties we 'expose' and how to use those to theme with those.

Ultimately, it doesn't matter where the definition lives as long as it is there.

@SanderElias
Copy link
Contributor

Oh, I have read it back, and I might be a bit unclear.
When we use CSS-custom-properties, we don't need to have any CSS file(s) available to users.
They can set the properties they want to override in their own system, and it will cascade down into our stuff.
Using the :where thing makes it easily overrulable.

@Harpush
Copy link
Collaborator Author

Harpush commented Apr 27, 2024

@SanderElias how about declaring the default variables using scss variables and each component or directive that needs them will import the variables file (library internal).
The usage in the component scss file for example will be: var(--name, $name).

It will still have zero effect on users and will make a bit more sense as there is a disconnected file just for variables.

The downside is the defaults which are used more than once will be duplicated after compilation and there are two names to use (scss one and css one).

The thing is I am not sure if a css variables file can be imported safely to all components and directives without duplicates or whether angular can include that file without end user intervention. Adding it in the angular split main component is a bit weird if we ever have other top level components as I guess it won't apply to them? And even if it will - it isn't very clear...

@SanderElias
Copy link
Contributor

The thing is I am not sure if a css variables file can be imported safely to all components and directives without duplicates or whether angular can include that file without end user intervention. Adding it in the angular split main component is a bit weird if we ever have other top level components as I guess it won't apply to them? And even if it will - it isn't very clear...

What do you mean by this? You don't need to import CSS variables. You define them once in the top component. (if we ever need to export multiple of those, we can deal with it then). That component needs to have ViewEncapsulation.none
From that point on, you can use them wherever you need them. That is how cascading works.
Mixing scss in makes it more complex indeed.

@Harpush
Copy link
Collaborator Author

Harpush commented Apr 27, 2024

If we put aside multiple top level components then putting it inside the main component does make sense.
I am not a fan of view encapsulation none but I don't mind going with it although I suspect it isn't needed but I need to make sure. It will also solve the ng deep problem.

Anyway I think the directive should be a component - the only question is when (this PR vs a later breaking PR).

@SanderElias
Copy link
Contributor

Anyway I think the directive should be a component - the only question is when (this PR vs a later breaking PR).
I would keep it separate unless it causes you to do a load of additional refactoring.

@Harpush
Copy link
Collaborator Author

Harpush commented Apr 28, 2024

@SanderElias
I started working on the change and it seems we have to decide:

  1. Use encapsulation none but apply strict rules to scss as there can be nested splitters (Only direct children > styling).
  2. Use emulated but ditch the renderer classes compared to angular class binding. The con is making the classes emulated and not global
  3. Use ng deep in a lot of places because classes are not scoped (due to renderer) which has problems similar to 1

@SanderElias
Copy link
Contributor

@Harpush I'm going to look into it, I'll let you know.

@SanderElias
Copy link
Contributor

@Harpush I did a quick test here: https://github.com/angular-split/angular-split/tree/test_css_encap_none.
I moved to CSS + viewEncap none. At first glance, everything seems to work okay, but I don't have time to look deeper.
I suspect that by doing that, we can loose most of the ng-deep in there too,
If not, that is not a big deal anymore because it is no longer deprecated. Still, it would be better to avoid using it.
Also, the ng-deep is an implementation detail now, especially if we add a couple of CSS-custom-properties to take care of the theming.

@Harpush
Copy link
Collaborator Author

Harpush commented Apr 29, 2024

@SanderElias I didn't say it won't work. And no need for ng-deep entirely.
I only said selectors in 99% of the cases must be declared with > as otherwise the styles will leak.
As most developers work with emulated - using regular nesting instead of > is more widely used and that is something that we need to pay attention to in following PRs.

I will push an updated version with encapsulation none.

@Harpush
Copy link
Collaborator Author

Harpush commented Apr 29, 2024

@SanderElias Are you sure :where works how you expect when we put it in split.component.scss? I might be missing something but :root get overridden and local css variables only affect top level split if they are nested.

@SanderElias
Copy link
Contributor

@SanderElias Are you sure :where works how you expect when we put it in split.component.scss? I might be missing something but :root get overridden and local css variables only affect top level split if they are nested.

Hmm, yes, I did this before, but I would use internal CSS vars and put in a fallback value in there.

example:

as-split {
  --_as-padding: var(--as-padding, 1rem);
  --_as-color: var(--as-color, gray)
  /* rest of styling */
}

/* can do, but not needed anymore: */
:root {
  :where(as-split) {
     --as-padding: 1rem;
     --as-color:  gray;
  }
 }

This makes it a bit easier for us I suspect. Wherever you need a custom thing, you use one of those --_as*
and it should properly cascade, even if you have a nested one, where the user overrides differently.
I can take a look on Friday if it doesn't do what you expect.

@Harpush
Copy link
Collaborator Author

Harpush commented Apr 29, 2024

The reason :where seems to not work id because each as-split redeclares it. I guess if it was declared globally once it would work (but we wanted to avoid changing the api surface).

The private variables will work though they too will be redeclared on each as-split when nested (not sure what's the impact here?)

@SanderElias
Copy link
Contributor

That impact will be minimal and will be inside the CSS layer of the browser, not affecting anything on the main thread.

@Harpush
Copy link
Collaborator Author

Harpush commented Apr 29, 2024

@SanderElias I pushed the changes. A few notes:

  1. I used private css vars and none encapsulation as suggested
  2. I used scss to ease the amount of repetitive css vars declarations
  3. I kept the iframe fix class change (minor private breaking)
  4. I used strict immediate child selectors
  5. I removed all the ng-deep
  6. There should be no breaking changes

@SanderElias
Copy link
Contributor

SanderElias commented Apr 30, 2024

@Harpush I reviewed your code, and do like most of it.
However, I really would like to get away from scss, and your PR adds more dependency on it.

Also, you have variable names that end with -bg-image-url which makes the names terrible long,
Perhaps just lose that part?
I think it makes sense to (in this, or in a future PR) add a export css style sheet where we define the available properties using the @property syntax,

That would look something like:

@property --as-gutter-icon-horizontal {
  syntax: "<URL>";
  inherits: true;
  initial-value:  
url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAeCAYAAADkftS9AAAAIklEQVQoU2M4c+bMfxAGAgYYmwGrIIiDjrELjpo5aiZeMwF+yNnOs5KSvgAAAABJRU5ErkJggg==');
}

Here is the CSS, without the need for additional processing:

as-split {
  --_as-gutter-background-color: var(--as-gutter-background-color, #eeeeee);
  --_as-gutter-icon-horizontal-bg-image-url: var(
    --as-gutter-icon-horizontal-bg-image-url,
    url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAeCAYAAADkftS9AAAAIklEQVQoU2M4c+bMfxAGAgYYmwGrIIiDjrELjpo5aiZeMwF+yNnOs5KSvgAAAABJRU5ErkJggg==')
  );
  --_as-gutter-icon-vertical-bg-image-url: var(
    --as-gutter-icon-vertical-bg-image-url,
    url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAB4AAAAFCAMAAABl/6zIAAAABlBMVEUAAADMzMzIT8AyAAAAAXRSTlMAQObYZgAAABRJREFUeAFjYGRkwIMJSeMHlBkOABP7AEGzSuPKAAAAAElFTkSuQmCC')
  );
  --_as-gutter-icon-disabled-bg-image-url: var(
    --as-gutter-icon-disabled-bg-image-url,
    url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAeCAYAAADkftS9AAAAIklEQVQoU2M4c+bMfxAGAgYYmwGrIIiDjrELjpo5aiZeMwF+yNnOs5KSvgAAAABJRU5ErkJggg==')
  );
  --_as-transition-duration: var(--as-transition-duration, 0.3s);
  --_as-gutter-disabled-cursor: var(--as-gutter-disabled-cursor, default);
}

as-split {
  display: flex;
  flex-wrap: nowrap;
  justify-content: flex-start;
  align-items: stretch;
  overflow: hidden;
  width: 100%;
  height: 100%;

  /* Add transition only when transition enabled + split initialized + not currently dragging. */
  &.as-transition.as-init:not(.as-dragging) {
    & > .as-split-gutter,
    & > .as-split-area {
      transition: flex-basis var(--_as-transition-duration);
    }
  }

  & > .as-split-gutter {
    border: none;
    flex-grow: 0;
    flex-shrink: 0;
    background-color: var(--_as-gutter-background-color);
    display: flex;
    align-items: center;
    justify-content: center;

    &.as-split-gutter-collapsed {
      flex-basis: 1px !important;
      pointer-events: none;
    }

    & > .as-split-gutter-icon {
      width: 100%;
      height: 100%;
      background-position: center center;
      background-repeat: no-repeat;
    }
  }

  & > .as-split-area {
    flex-grow: 0;
    flex-shrink: 0;
    overflow-x: hidden;
    overflow-y: auto;

    &.as-hidden {
      /* When <as-split-area [visible]="false"> force size to 0. */
      flex: 0 1 0px !important;
      overflow-x: hidden;
      overflow-y: hidden;
    }

    & > .as-iframe-fix {
      position: absolute;
      top: 0;
      left: 0;
      width: 100%;
      height: 100%;
    }
  }

  &.as-horizontal {
    flex-direction: row;

    & > .as-split-gutter {
      flex-direction: row;
      cursor: col-resize;
      /* Fix safari bug about gutter height when direction is horizontal. */
      height: 100%;

      & > .as-split-gutter-icon {
        background-image: var(--_as-gutter-icon-horizontal-bg-image-url);
      }
    }

    & > .as-split-area {
      height: 100%;
    }
  }

  &.as-vertical {
    flex-direction: column;

    & > .as-split-gutter {
      flex-direction: column;
      cursor: row-resize;
      width: 100%;

      & > .as-split-gutter-icon {
        background-image: var(--_as-gutter-icon-vertical-bg-image-url);
      }
    }

    & > .as-split-area {
      width: 100%;

      &.as-hidden {
        max-width: 0;
      }
    }
  }

  &.as-disabled {
    & > .as-split-gutter {
      cursor: var(--_as-gutter-disabled-cursor);

      & > .as-split-gutter-icon {
        background-image: var(--_as-gutter-icon-disabled-bg-image-url);
      }
    }
  }
}

(if you prefer I can push this change into this branch?)

@Harpush
Copy link
Collaborator Author

Harpush commented Apr 30, 2024

@SanderElias May I ask why are you against scss? For public API I understand as some people don't wish to use it. But why not internally?
The main benefits are:

  1. Reuse code with features not yet in css
  2. Some large companies use older chrome versions and css native nesting is available for newer versions of chrome only. If I remember correctly scss still compiles to the non nested version. Unless angular does some magic behind to transform css files too.

Concerning shortening the names - I like the as-gutter-icon-horizontal so will change.

Concerning the css properties - generally a good idea too but where to put it? You mentioned export css but who will import it? If the user I guess that's a breaking change? Also is there a way to define syntax for cursor?

@SanderElias
Copy link
Contributor

@Harpush Because SCSS doesn't add anything anymore, it just takes additional build time.

  1. Reuse code with features not yet in CSS

Name one. I wouldn't know about any that would have an impact. And when that is the case, we should utilize POSTCSS anyway.

  1. Some large companies use older Chrome versions, and css native nesting is available for newer versions of Chrome only. If I remember correctly scss still compiles to the non nested version. Unless angular does some magic behind to transform css files too.

Hmm, Angular does, based on the browsersList. But aside from that, it has been chrome since V112, and without the & since 120 (4 versions ago)

Concerning the css properties - generally a good idea too but where to put it? You mentioned export css but who will import it? If the user I guess that's a breaking change? Also is there a way to define syntax for cursor?

It should be in a CSS file we distribute. People who want to use the theme's can then optionally import this. If they don't everything will still work as expected. However authoring the CSS where you want to set our vars would benefit.

@Harpush
Copy link
Collaborator Author

Harpush commented Apr 30, 2024

@SanderElias
Mostly functions and mixins... Pay attention I used it in this PR and a new variable requires less repetition now (writing twice instead of three times and no need for --as- or --_as-).
Fair enough concerning the browserlist support.
So a new CSS file which will be used inside angular.json styles property? Or imported similar to SCSS (using @import)? Unfortunately I am not really familiar with the new css (and non SCSS) flavors.
And lastly what will be the syntax type for cursor? Didn't find any built in type for it.

@SanderElias
Copy link
Contributor

I would say it can be used with @import where needed. Adding it to angular.json is unnecessary, as it will only add noise when not used.
And if there is no syntax type for the cursor thing, don't create a @property rule for it, as it doesn't add any value.

@Harpush
Copy link
Collaborator Author

Harpush commented Apr 30, 2024

@SanderElias So I made a little test. Seems this can work:

  1. ng-package.json
    image
  2. package.json
    image
  3. _theme.css
    image
  4. app styles.scss
    image

What do you think? The only downside is the initial value (default). In @property initial value is required and then the user must import the style. If we remain with fallback like now we have default duplication.
That also can make this PR a breaking one (or create a different PR).

@SanderElias
Copy link
Contributor

@Harpush The fallback needs to be in the component anyway! When a browser doesn't support @property yet, there will be no default value otherwise.
I agree that it's not ideal to define them twice, but that is the best solution for now.
I like how this is shaping up.

@Harpush
Copy link
Collaborator Author

Harpush commented May 1, 2024

I reverted back to the default values in the component.
Unfortunately that means we declare the variable name 4 times and the default value twice.
I really think we should solve it asap in a following PR (either back to SCSS or another solution) as currently DX suffers greatly for the library contributors.

@Jefiozie
Copy link
Member

Jefiozie commented Jun 2, 2024

@SanderElias this one can be merged,right?

@SanderElias SanderElias merged commit d011162 into angular-split:main Jun 3, 2024
5 checks passed
@SanderElias
Copy link
Contributor

Yes

@Harpush Harpush deleted the add-theming-support branch June 21, 2024 07:23
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.

Add scss theme for customization Publish styles as separate scss files to allow for easy customization
4 participants