-
Notifications
You must be signed in to change notification settings - Fork 215
feat: modernize angular split #444
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
feat: modernize angular split #444
Conversation
Is it possible to add more scss variables with default value in order to customize:
Or to use css variable, it can also work :) |
That's unrelated to this PR and already got merged in #367 |
@SanderElias @Jefiozie the merge conflicts are only due to CSS variables PR. |
Anxious to see this new version. I was in the process of updating my dashboard app to try and include cdk drag & drop. I'll not waist time and wait a bit so I'll be available as a beta tester if needed. Thanks for all the great work you are doing!!! |
I currently use [order] on split areas that are included/excluded from the dom with an @if/ngIf. Without setting the order, the areas included by the @if/ngIf always get rendered after the static area(s). Would this PR break this functionality? Example:
|
With this PR the order is based on DOM order. That means if the ngIf adds the area before the static ones it will work as expected. |
will avoid multiple warnings in console and the main/max size won't suddenly start working while dragging
…cts sizes" This reverts commit 5fc3aa9.
@Jefiozie Would you have time to review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some questions but nothing serious. My main concern is that I would like to have a deprecation notice before we remove the old types OR we need to have a schematic in place to migrate everything.
Im not against either one but backwars compatibily is something i would appriciate.
@@ -35,32 +35,32 @@ context('Gutter click example page tests', () => { | |||
|
|||
it('Click gutters to switch area sizes between 0 and X', () => { | |||
cy.get('.as-split-gutter').eq(0).click() | |||
cy.wait(1500) | |||
cy.wait(2000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WHy is this change needed for cypress?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't seem to figure out how it worked before. The transition is 1.5s and the delay is 1.5s. When running live it works well but fails in CI run.
At 1500 it fails on 9 pixels instead of 0
At 1600 it fails on 5 pixels instead of 0
At 2000 it passes
Not sure what to do about it...
@@ -62,10 +58,14 @@ | |||
"@types/node": "20.5.4", | |||
"@typescript-eslint/eslint-plugin": "6.19.0", | |||
"@typescript-eslint/parser": "6.19.0", | |||
"cypress": "12.17.4", | |||
"angular-cli-ghpages": "^2.0.0-beta.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a none beta version or is there a issue with this version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great question. I don't know why this is beta. If you look at line 43 before my changes it was beta to begin with. The cypress update just moved those dependencies around (ABC order)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make the old types depracted and not delete them straight away? It would be nice if we could make it so that people can use both for a period of time.
What do you think of this?
@Jefiozie Concerning deprecation and migrations - let's review what needs discussion:
If we go the schematics direction - we need to create the required infra as there is no schematics collection project in this repo. |
When will this be available? |
@Jefiozie I wrote some considerations above - can you share your opinion on them? |
@Harpush I actually am using the collapse/expand for snapping like in the vscode. Can you please maybe add a snap option? I had to sortof implement it with the collapse/expand feature. Don't remove it please. |
Can you please elaborate how you would expect the snap to work? Visible vs |
Is this great upgrade planned to be available soon? Can't wait to try it out! |
@Harpush we've decided to leave it as is and merge it. We will do a beta release of the package soon. |
In that case how about an RFC on a snap feature to replace expand/collapse? |
That is a good idea, would you be interested in writing the first draft/ iteration? |
Sure I have something in mind that should also answer most of the collapse/expand intended usage hopefully. |
Hi everyone, We just released a beta version of angular-split, you can find it angular-split 18.0.0-beta.0 |
@Harpush How can I programmatically change the sizes? Without using the mouse. What should I use instead of collapse/expand/ |
Option 1: like the custom gutter demo use hide or |
@Harpush Why did you change I was asked to make a PR, but before doing anything, I want to know the rationale. |
@Akxe split area has both css and html template. It also affects the host styles and classes. Making it a directive will require to use DOM apis to emulate html and css. If you think adding the directive to your component will work and won't break anything you can create a PR that changes the split area component selector to support attribute selector while keeping it a component. |
Features
fraction
unit type for easy1/3
size splitting and maybe will allow mixing percent and pixels sizes too.provideX
pattern and adds type safetyI
which isn't recommended for typescriptBreaking(ish) changes
Breaking changes
Tests
Missing features (require discussion)
Out of scope
I might have forgotten some features or breaking changes....
Closes #135
Closes #426
Closes #378
Closes #439
Closes #433
Closes #267