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

mapp.ui.elements.alert/ confirm methods #1484

Merged
merged 42 commits into from
Sep 27, 2024

Conversation

cityremade
Copy link
Member

@cityremade cityremade commented Sep 18, 2024

mapp.ui.elements.alert
mapp.ui.elements.confirm

This is alert / confirm elements to display information to the user.

Uses the existing ui.elements.dialog.

2 parameters added to the dialog:

closeOnClick - when set to true the dialog closes when the document is clicked anywhere over the backdrop.
onClose - callback function to execute when dialog fires a close event.

mapp.ui.elements.alert({
   text: "Drivetimes have been created."
})


---
- To see the specific tasks where the Asana app for GitHub is being used, see below:
  - https://app.asana.com/0/0/1208407306090518

@cityremade cityremade added the Feature New feature requests or changes to the behaviour or look of existing application features. label Sep 18, 2024
@cityremade cityremade self-assigned this Sep 18, 2024
@cityremade cityremade linked an issue Sep 18, 2024 that may be closed by this pull request
@cityremade cityremade marked this pull request as ready for review September 18, 2024 13:37
Copy link
Member

@dbauszus-glx dbauszus-glx left a comment

Choose a reason for hiding this comment

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

This should make use of the mapp.ui.elements.dialog() method to create the dialog itself.

Alert now uses dialog element.
Dialog element now supports 2 new parameters: closeOnClick - to clear the dialog when the element or backdrop are clicked
onClose - optional callback function to run on dialog close event.
@cityremade
Copy link
Member Author

cityremade commented Sep 23, 2024

Changes made:

alert is now a ui.elements.dialog.

2 parameters added to the dialog:

closeOnClick - when set to true the dialog closes when the document is clicked anywhere over the backdrop.
onClose - callback function to execute when dialog fires a close event.

Both documented in the dialog module.

@cityremade
Copy link
Member Author

Confirm method now in the same pr;

Copy link
Member

@dbauszus-glx dbauszus-glx left a comment

Choose a reason for hiding this comment

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

The confirm dialog in /ui/locations/view breaks with mapp.ui.elements not being a function.

image

This can be tested by removing a location with outstanding edits.

.mapp-says should be replaced with a semantic class like .alert-confirm

The jsdoc return for function that return a promise must be promise with the text explaining how the promise resolves.

lib/ui/locations/view.mjs Outdated Show resolved Hide resolved
@RobAndrewHurst
Copy link
Contributor

I have added in a test for the dialog ui element to test the close functionality. And I noticed after invoking the close that the dialog can still be found in the DOM. I am not too sure if this is something todo with dialogs, or if we are not closing correctly.

Copy link
Contributor

@RobAndrewHurst RobAndrewHurst left a comment

Choose a reason for hiding this comment

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

See comment above.

@cityremade
Copy link
Member Author

ui.elements.dialog has onClose callback where closed dialog can be removed if needed.
Removing the element will not be a case at all times.

Copy link
Member

@dbauszus-glx dbauszus-glx left a comment

Choose a reason for hiding this comment

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

The dialog element method decorates the params argument.

const dialog = {
  content: 'foo'
}

// dialog becomes the decorated dialog object with the dialog.node being the dialog node element.
mapp.ui.elements.dialog(dialog)

The dialog element method should return the decorated dialog.

// dialog becomes the decorated dialog with the dialog.node being the dialog node element.
const dialog = mapp.ui.elements.dialog({
  content: 'foo'
})

The dialog.close() method should call the dialog element close method as well as remove the element.

dialog.close = ()=>{
  dialog.node.close()
  dialog.node.remove()
}

Copy link
Member

@dbauszus-glx dbauszus-glx left a comment

Choose a reason for hiding this comment

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

The confirm dialog closes when I click on the header. This shouldn't happen.

The closeBtn shouldn't be there if it shouldn't be there. Regardless whether the styling set's the button to the same colour.

@dbauszus-glx
Copy link
Member

We try to avoid negating flags. I removed the hideCloseBtn reference. Default will be no close button. If the closeBtn is defined, ie true then the property will be mutated to a html fragment. No web api node needed here. Also no tertiary statement needed.

7bb0b49

@dbauszus-glx
Copy link
Member

There was a lot of unnecessary stopPropagation. There must not be a click event on the dialog element. Also the dialog must not close when clicked anywhere. That defeats the logic of a confirm dialog. A confirm dialog should be boolean. E.g. Ok=true, Cancel=false. There is no tertiary whatever/close state.

There was a lot of close onClose closeDialog action. This has been streamlined.

The closeDialog method calls the onClose event method.

The dialog.close is closeDialog method.

dialog.clkoseBtn calls closeDialog method.

dialog onclose event calls closeDialog method.

there shouldn't be any stopPropagation required.

image

Copy link
Member

@dbauszus-glx dbauszus-glx left a comment

Choose a reason for hiding this comment

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

This is now working for me as expected. Please see comments.

Copy link

sonarcloud bot commented Sep 27, 2024

@RobAndrewHurst RobAndrewHurst merged commit 1f35e91 into GEOLYTIX:main Sep 27, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature requests or changes to the behaviour or look of existing application features. v4.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Framework to have its own alert and confirm dialogs
4 participants