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

refactor(ui5-dialog): rename "open()" to "show()" #3528

Merged
merged 5 commits into from
Jul 24, 2021
Merged

Conversation

ilhan007
Copy link
Member

@ilhan007 ilhan007 commented Jul 21, 2021

As part of the Component API Review we are looking for providing a declarative way to open/close popups with attribute.
Most probably the attribute will be called "open". The requirement has been raised from different sources. It was discussed internally in order to make our popups closer to the native spec (HTMLDialogElement Spec). But also, UI5 Web Components 4 react and our community asked for the declarative API.
There is already a BLI for that: #3527 that will arrive in future.

But before that, we need to make sure the declarative API won't conflict with the existing APIs. And, this is the goal of the current PR.
The Popover has "openBy", we recently changed ResponsivePopover's "open" to "openBy", so the Dialog left with its public method "open".

Main change: Dialog's public method "open" is renamed to "show" to allow introducing a "open" attribute in future.

Other Changes:

  • Popup class: private "open" renamed to "_open" (to avoid any conflicts with the future "open" attribute)
  • Popup class protected "show" renamed to "_show" (as "show" is the Dialog's public method now)

BREAKING CHANGE: Dialog's "open" method has been renamed to "show"

@ilhan007
Copy link
Member Author

ilhan007 commented Jul 21, 2021

Hello @SAP/ui5-webcomponents-topic-rd not sure if this particular item has been discussed with you.

As part of the Component API Review we agreed to introduce declarative API to open/close popups (Popover, Responsive Popover, Dialog). The requirement has been raised from different sources. It was discussed internally in order to make our popups closer to the native spec (HTMLDialogElement Spec). But also, UI5 Web Components 4 react and our community asked for the declarative API.

The goal of the current PR is to prepare for this step by ensuring the current API does not conflict with the future "open" attribute that we foresee. Therefore some of the methods has been renamed. The most important change is the renaming of the public method Dialog#open to called Dialog#show. This way we can add "open" attribute to the Dialog and Popover in future without breaking anything.

<ui5-dialog open />

<ui5-button id="info">Info</ui5-button>
<ui5-popup open opener="info"></ui5-popup>

If you have any concerns @SAP/ui5-webcomponents-topic-rd @TeodorTaushanov @gmkv @dimovpetar @alexandar-mitsev, please just let us know and we can have a meeting to discuss if this is the right way forward.

@alexandar-mitsev
Copy link
Contributor

alexandar-mitsev commented Jul 22, 2021

Hi @ilhan007 ,

It looks ok.
I was thinking if we should also change openBy to showBy for consistency. But maybe it is not really needed.

Regards

@ilhan007
Copy link
Member Author

@alexandar-mitsev it is a valid point.

Also, if we want to add open-by(openBy) as attr(property), then we definetly should rename the openBy method. But we thought "opener" is fine for that.

However, "showBy" is more consostent with "show".
I will check also what the others in my team think about it.

@ilhan007 ilhan007 merged commit 0ecc508 into master Jul 24, 2021
@ilhan007 ilhan007 deleted the dialog-ref branch July 24, 2021 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants