goetzrobin / spartan

Cutting-edge tools powering Angular full-stack development.
https://spartan.ng
MIT License
1.31k stars 137 forks source link

cannot disable button programatically in brmMenu -> brmMenuGroup -> brnMenuItem #60

Closed jkuri closed 10 months ago

jkuri commented 10 months ago

Please provide the environment you discovered this bug in.

<button type="button" hlmBtn size="sm" variant="ghost" [brnMenuTriggerFor]="pipelineMenu">
  <hlm-icon name="radixDotsHorizontal" size="sm" />
</button>
<ng-template #pipelineMenu>
  <div hlm brnMenu class="w-56">
    <div brnMenuGroup>
      <button brnMenuItem [disabled]="true"> <!-- HERE -->
        <hlm-icon name="radixCross2" hlmMenuIcon />
        <span>Delete Pipeline</span>
        <hlm-menu-shortcut>⌘D</hlm-menu-shortcut>
      </button>
    </div>
  </div>
</ng-template>

Which area/package is the issue in?

dropdown-menu

Description

Hi, first of all - thanks for your amazing work! We are already using some of the components in production environment no matter this components are still in alpha ;-)

As I provided example above, I cannot disable button with brnMenuItem directive applied programatically using the [disabled]="true" for example. Note that if I use just disabled html property it works as expected; like that:

<button brnMenuItem disabled></button> <!-- this works -->

but not:

<button brnMenuItem [disabled]=""true"></button> <!-- this doesn't work -->

This is not a blocker, I use a workaround to display disabled button conditionally with *ngIf, but need to render same button twice.

Please provide the exception or error you saw

No exception or error is thrown for this behavior, it's also not expected.

Other information

No response

I would be willing to submit a PR to fix this issue

goetzrobin commented 10 months ago

What version are you using?

jkuri commented 10 months ago

I am using version 0.0.1-alpha.309, will now try to upgrade to 0.0.1-alpha.311. I noticed that the button is actually disabled, but it doesn't inherit disabled styles with opacity applied.

jkuri commented 10 months ago

I tried now with 0.0.1-alpha.311 and issue still persists. I had some issues with peer dependencies before getting it to work with the new version, I think it's not okay to specify peer dependencies to exact version in semver like 17.0.2 but to ^17.0.0. For example I updated my app with ng update @angular/core @angular/cli and it updated it to version 17.0.4 which is not exact version as 17.0.2 which is specified in @spartan-ng/ui-menu-brain latest version;

"peerDependencies": {
  "@angular/core": "17.0.2",
  "@angular/cdk": "17.0.0"
}

but, I think it should be;

"peerDependencies": {
  "@angular/core": "^17.0.0",
  "@angular/cdk": "^17.0.0"
}

Can I open a PR for this? Otherwise other users may have issues updating to latest in the future.

Thanks, Jan

goetzrobin commented 10 months ago

Yes. I am still trying to find a better way to manage dependencies in a better way! I am using an eslint plugin right now to sort of automate it. If you take a look at the package.json scripts that should give you an idea of what I am doing.

Let me know if that makes sense! Looking forward to the PR

goetzrobin commented 10 months ago

I am using version 0.0.1-alpha.309, will now try to upgrade to 0.0.1-alpha.311. I noticed that the button is actually disabled, but it doesn't inherit disabled styles with opacity applied.

Interesting! I'll see if that's a class binding issue!

goetzrobin commented 10 months ago

@jkuri I found the issue. Releasing a new version now that should have the fix!

goetzrobin commented 10 months ago

@jkuri 0.0.1-alpha.313 is now available on NPM with the fix. Let me know if it worked 👍

jkuri commented 10 months ago

I updated to 0.0.1-alpha.313 and confirming that it works now 👍 thanks for the quick response and fix.