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

ShellBar: Inconsistent Event Handler #297

Closed
MarcusNotheis opened this issue Apr 2, 2019 · 3 comments · Fixed by #298
Closed

ShellBar: Inconsistent Event Handler #297

MarcusNotheis opened this issue Apr 2, 2019 · 3 comments · Fixed by #298
Assignees
Labels
approved This issue is approved bug This issue is a bug in the code

Comments

@MarcusNotheis
Copy link
Collaborator

MarcusNotheis commented Apr 2, 2019

Describe the bug
A clear and concise description of what the bug is.
According to the documentation, the events notificationsPress, productSwitchPress, profilePress and titlePress should all have the detail iconRef.
This seems not the be the case at the moment.

Current Content of event.detail:

  • titlePress: iconRef ✅
  • productSwitchPress: detail
  • profilePress: iconRef ✅
  • notificationsPress: null

To reproduce
Steps to reproduce the behavior:

  1. Open the playground URL: https://sap.github.io/ui5-webcomponents/playground/components/ShellBar/
  2. Open the development tools and insert the following snippet:
    var shellbar = document.querySelector('#shellbar');
    shellbar.addEventListener('notificationsPress', (e) => console.log(e.type, e.detail))
    shellbar.addEventListener('productSwitchPress', (e) => console.log(e.type, e.detail))
    shellbar.addEventListener('profilePress', (e) => console.log(e.type, e.detail))
    shellbar.addEventListener('titlePress', (e) => console.log(e.type, e.detail))
  3. Click on the respective icons in the first Shell bar and look at the console output:
    productSwitchPress {detail: ui5-icon#__el26-item4..sapWCShellBarIconButton.sapWCShellBarIconProductSwitch}
    profilePress {iconRef: div#__el26-item-3..sapWCShellBarImageButton.sapWCShellBarIconButton}
    notificationsPress null
    titlePress {iconRef: button.sapWCShellBarMenuButton}

Expected behavior
A clear and concise description of what you expected to happen.
I would expect all details to contain iconRef

Context

  • UI5 Web Components version: 0.9.0
  • OS/Platform: macOS 10.14.4
  • Browser (if relevant): Chrome 73.0.3683.86
  • Other information: {...}

Affected components (if known)
ui5-shellbar

@MapTo0
Copy link
Member

MapTo0 commented Apr 3, 2019

Hello @MarcusNotheis

While fixing this gap I was thinking that it would be good if we define better names for ref parameter in in these events. Such as targetRef or something like that.

In the end it is not always an icon clicked there.

What do you think?

@MapTo0 MapTo0 added approved This issue is approved bug This issue is a bug in the code labels Apr 3, 2019
@MarcusNotheis
Copy link
Collaborator Author

Hi @MapTo0,

yes, totally see your point.
targetRef sounds good to me.

Thanks for looking into that!

@MarcusNotheis
Copy link
Collaborator Author

If you are changing the ShellBar anyways it would be really nice if you could add events for logoPress and coPilotPress too.

MapTo0 pushed a commit that referenced this issue Apr 4, 2019
BREAKING CHANGE: press handlers used to have param "iconRef". As not always
an Icon is pressed (e.g. title is not an icon) - the parameter is renamed to targetRef.

Fixes: #297
MapTo0 added a commit that referenced this issue Apr 4, 2019
BREAKING CHANGE: press handlers used to have param "iconRef". As not always
an Icon is pressed (e.g. title is not an icon) - the parameter is renamed to targetRef.

Fixes: #297
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This issue is approved bug This issue is a bug in the code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants