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

feat(ui5-barcode-scanner-dialog): add 'open' property #8316

Merged
merged 7 commits into from
Mar 13, 2024

Conversation

kgogov
Copy link
Member

@kgogov kgogov commented Feb 20, 2024

This change introduces a new open property to the ui5-barcode-scanner-dialog component, enhancing the control over the dialog's visibility. This feature aligns with the behavior of other similar components, providing a more consistent user experience.

Related: #8072

This change introduces a new `open` property to the
`ui5-barcode-scanner-dialog` component, enhancing the control over the
dialog's visibility. This feature aligns with the behavior of other
similar components, providing a more consistent user experience.

Related: #8072
@kgogov kgogov marked this pull request as ready for review February 26, 2024 20:46
Copy link
Contributor

@plamenivanov91 plamenivanov91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the .hbs file add this to the dialog

?open="{{_open}}"

Try to add some tests with the newly added code (might simply try the method and the new prop if they get too complex for real-life flow).

packages/fiori/src/BarcodeScannerDialog.ts Outdated Show resolved Hide resolved
packages/fiori/src/BarcodeScannerDialog.ts Show resolved Hide resolved
@plamenivanov91
Copy link
Contributor

plamenivanov91 commented Mar 11, 2024

In the .hbs file add this to the dialog

?open="{{_open}}"

Try to add some tests with the newly added code (might simply try the method and the new prop if they get too complex for real-life flow).

By the way with the code I suggested, the this.dialog.close(); and this.dialog.show(); become redundant (in BarcodeScannerDialog.ts).

@kgogov
Copy link
Member Author

kgogov commented Mar 12, 2024

In the .hbs file add this to the dialog
?open="{{_open}}"
Try to add some tests with the newly added code (might simply try the method and the new prop if they get too complex for real-life flow).

By the way with the code I suggested, the this.dialog.close(); and this.dialog.show(); become redundant (in BarcodeScannerDialog.ts).

I considered your suggestion to add open attribute in the template but I've encountered the following:

  • According to the sequence of actions, we need to first request permission from the user to use the camera and then decide whether to open a dialog. Actually, in our case, whether we put an open attribute in the template file is irrelevant, since we use internal methods for the purpose. I tested both cases, noticing that in the first case when we don't put an open attribute in the template, in _showDialog() we assign a dialog to this.dialog with open property set to false which is correct, but in the second case we receive a dialog with open property already set to true, which may be not correct, because technically we have not called this.dialog.show();
  • I'm not so sure that we can remove the logic to show and close the internal managed dialog, because if we do we risk missing the check for user rights to use the camera.
  • We also have to think about backwards compatibility.

@kgogov kgogov merged commit 8f59d16 into SAP:main Mar 13, 2024
9 checks passed
@kgogov kgogov deleted the barcode-scanner-dialog branch March 13, 2024 11:33
nnaydenow pushed a commit that referenced this pull request Mar 20, 2024
* feat(ui5-barcode-scanner-dialog): add 'open' property

This change introduces a new `open` property to the
`ui5-barcode-scanner-dialog` component, enhancing the control over the
dialog's visibility. This feature aligns with the behavior of other
similar components, providing a more consistent user experience.

Related: #8072
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants