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

Switch: In popup menu stops working after 10.1.1 #1127

Closed
ahnpnl opened this issue Sep 15, 2021 · 15 comments · Fixed by infor-design/enterprise#5674
Closed

Switch: In popup menu stops working after 10.1.1 #1127

ahnpnl opened this issue Sep 15, 2021 · 15 comments · Fixed by infor-design/enterprise#5674

Comments

@ahnpnl
Copy link
Contributor

ahnpnl commented Sep 15, 2021

Describe the bug
Switch no longer works in soho-popupmenu after version 10.1.1 The switch can't be toggled on/off anymore.

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://github.com/ahnpnl/soho-bug-repo
  2. Use master branch
  3. Start server with ng serve
  4. Open http://localhost:4200
  5. Click on the button with the text "Click Me". After that a popup menu item appears.
  6. Click on the switch inside the popup menu. Observe that the switch can be toggled properly (the 1st time the popup menu might disappear but you just need to click on the button again to open the popup menu)
  7. Now switch to https://github.com/ahnpnl/soho-bug-repo/tree/switch-10.2.1 , https://github.com/ahnpnl/soho-bug-repo/tree/switch-10.3.1 , https://github.com/ahnpnl/soho-bug-repo/tree/switch-latest
  8. Repeat the same above steps
  9. Observe that the switch can no longer be toggled properly, not like master branch.

Expected behavior
The switch should be toggled.

Version

  • ids-enterprise-ng: 10.2.1, 10.3.1, latest

Screenshots
N.A.

Platform

  • Device (if applicable) N.A.
  • OS Version: Windows 10, MacOS Big Sur
  • Browser Name: Google Chrome
  • Browser Version: Version 93.0.4577.82 (Official Build) (x86_64)

Additional context
N.A.

@ahnpnl ahnpnl changed the title regression: switch in popup menu stops working after 10.1.1 Regression: switch in popup menu stops working after 10.1.1 Sep 15, 2021
@ahnpnl ahnpnl changed the title Regression: switch in popup menu stops working after 10.1.1 Regression 💥 switch in popup menu stops working after 10.1.1 Sep 15, 2021
@tmcconechy tmcconechy added [3] Velocity rating (Fibonacci) type: bug 🐛 labels Sep 15, 2021
@tmcconechy
Copy link
Member

Having a hard time with this example. It worked the first time but then i went to latest. And then it will start but the page http://localhost:4200/ wont load. Cannot GET /

So not seeing this example not work. You can try a stackblitz? All i really need to see is it not working on the latest version.

Also your markup looks a little different than the examples https://github.com/infor-design/enterprise-ng/blob/main/src/app/checkbox/checkbox.demo.html#L25-L28 so i would try that and use that in particular the field and the switch setting.

What i would check for is if what you clicking on is the label and its not out of position or something.
Screen Shot 2021-09-15 at 10 08 01 AM

Could be related to this fe2e346 cc @bthharper

@tmcconechy tmcconechy added this to Triage in Enterprise (Next) Sprint Grooming via automation Sep 15, 2021
@tmcconechy tmcconechy changed the title Regression 💥 switch in popup menu stops working after 10.1.1 Switch: In popup menu stops working after 10.1.1 Sep 15, 2021
@tmcconechy
Copy link
Member

Ok, update: i can see the issue on https://github.com/ahnpnl/soho-bug-repo/tree/switch-latest but still there is a difference in the markup. So maybe refactoring to that will work.

Dont know why but its not recognizing the [switch] option in this example.

@ahnpnl
Copy link
Contributor Author

ahnpnl commented Sep 26, 2021

I tried using the new markup but still it doesn't solve the problem. The problem happens only when using switch inside popup menu. The switch works fine if putting outside popup menu.

@tmcconechy
Copy link
Member

We found this issue which has a PR infor-design/enterprise#5652 we could try with that branch on?

@tmcconechy
Copy link
Member

ok so its not that but it might be one of these?
Screen Shot 2021-09-27 at 10 11 41 AM

Is it possible to change this to show the code? Or maybe we just need a few files and can drop it in enterprise-ng to reproduce? Or make an EP example?

One thing i'm thinking is maybe you can use a popdown instead of a popupmenu? As the popupmenu is usually for menu items and maybe the click is getting "stolen".

@ahnpnl
Copy link
Contributor Author

ahnpnl commented Sep 27, 2021

ok so its not that but it might be one of these?
Screen Shot 2021-09-27 at 10 11 41 AM

Is it possible to change this to show the code? Or maybe we just need a few files and can drop it in enterprise-ng to reproduce? Or make an EP example?

To reproduce in EP example, you can go to https://github.com/infor-design/enterprise/blob/main/app/views/components/button/example-menubutton.html#L16 and add another <li> like below

<li>
          <div class="switch field">
            <input class="switch" type="checkbox" id="cpf-switch-setting" />
            <label for="cpf-switch-setting" />
          </div>
</li>

and the problem can be reproduced with EP

One thing i'm thinking is maybe you can use a popdown instead of a popupmenu? As the popupmenu is usually for menu items and maybe the click is getting "stolen".

I will try this in the meantime.

@tmcconechy
Copy link
Member

Yeah i can see the issue is we added preventDefault for menu clicks so that in Angular they wouldnt navigate. So its taking the switch click away. I think its fixable with an exception checking for switch. But popupmenu should probably just be for menus. So do try popdown meanwhile

@ahnpnl
Copy link
Contributor Author

ahnpnl commented Sep 28, 2021

With popdown, switch works in it but the style doesn't really fit with my use case
image

comparing to switch in a popup
image

Besides, in my use case, I'm using soho-toolbar-flex with soho-toolbar-flex-more-button and I have the switch in it.

@ahnpnl
Copy link
Contributor Author

ahnpnl commented Sep 28, 2021

Ok I found the issue. The issue is caused by this line https://github.com/infor-design/enterprise/blob/4.55.3/src/components/popupmenu/popupmenu.js#L1026

and this line has been removed in infor-design/enterprise@cdf007e#diff-a13b4c7e75becf0d4ac1a993ae44c33986e95c1b864f35a1eb305509c2e64724 which fixes this issue. The change is available in main but not yet in any releases.

Feel free to close this issue. It would be nice if there can be a release for 10.6.x to fix this problem.

@tmcconechy
Copy link
Member

ok great. regarding the popdown. you would have to just format the height and width with css. It is maybe the right component for this.

But good we fixed this issue. We will probably be releasing 10.7.x in about a week and im not 100% sure which change set i would patch back to 10.6.x. Can you wait for 10.7.x? If not I can probably track it down....

@ahnpnl
Copy link
Contributor Author

ahnpnl commented Sep 28, 2021

Thanks for the information. I will tell my colleagues to wait for 10.7.x. Our team only has this week to perform an upgrade so I guess that's not possible from your side for a patch to 10.6.x

@tmcconechy
Copy link
Member

I could but this gets a little confusing because after infor-design/enterprise@cdf007e#diff-a13b4c7e75becf0d4ac1a993ae44c33986e95c1b864f35a1eb305509c2e64724 we did infor-design/enterprise#5652 as that fix broke a lot of things

If you test on main branch and that works i guess i could just components/popupmenu/popupmenu.js back to 4.55 (10.6.x). But make sure that works on main? that would be the main thing.

A patch takes me about 30 min of work...

@ahnpnl
Copy link
Contributor Author

ahnpnl commented Sep 28, 2021

we are not in a hurry so any ways are fine :)

@tmcconechy
Copy link
Member

ok good. lets wait. Also because of what i just said i wasnt sure if it was actually fixed because i reverted that fix. So I made a fix on this PR https://github.com/infor-design/enterprise/pull/5674/files

@tmcconechy tmcconechy moved this from Pending Review to Ready for QA (beta) in Enterprise 4.56.x (Sept 2021) Sprint Sep 29, 2021
@tmcconechy tmcconechy moved this from Ready for QA (beta) to Failed QA (beta) in Enterprise 4.56.x (Sept 2021) Sprint Sep 30, 2021
@tmcconechy tmcconechy moved this from Failed QA (beta) to Ready for QA (beta) in Enterprise 4.56.x (Sept 2021) Sprint Sep 30, 2021
@CindyMercadoReyes
Copy link

This issue is now resolved.

@CindyMercadoReyes CindyMercadoReyes moved this from Ready for QA (beta) to Done in Enterprise 4.56.x (Sept 2021) Sprint Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: regression bug ↩️ [3] Velocity rating (Fibonacci)
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants