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

Review ui.elements.modal #1100

Merged
merged 7 commits into from
Feb 2, 2024
Merged

Conversation

dbauszus-glx
Copy link
Member

@dbauszus-glx dbauszus-glx commented Jan 30, 2024

Modal must not have close button if not defined in constructor.

Modal must not have header if not defined in constructor.

Modal must not have inline styles if not defined in constructor.

Modal should take size from content.

Modal should be movable within the context of the parent.

A target in which the modal will be positioned absolute must be defined.

A header is optional. Can be provided as string or html element.

Close is optional. A close button will be displayed in the top right corner.

Default position is top: 0, left: 0.

Content must be provided to calculate the offset width and height.

mapp.ui.elements.modal({
  target: retailplaceMapview.target,
  header: 'Foo',
  close: true,
  top: 20 // pixel, default 0
  right: 20 // pixel, default left: 0
  content: layerSwitch,
  css_style: 'padding: 10px;',
  contained: true,
  //containedCentre: true
})

With the contained:true flag the modal bounds can not be shifted outside the target bounds.

With the containedCentre:true flag the centre of the modal can not be shifted outside the target bounds.

@dbauszus-glx dbauszus-glx added the Feature New feature requests or changes to the behaviour or look of existing application features. label Jan 31, 2024
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.

Request for change ✍️

After popping out a style into a modal and then closing the style, you have to select another style before being allowed to pop out or see the style again.

I think expected behaviour would be that the legend is returned to the layers panel.

Screen.Recording.2024-02-01.at.09.47.29.mov

@dbauszus-glx
Copy link
Member Author

@RobAndrewHurst The issue with close method should be fixed now.

@RobAndrewHurst
Copy link
Contributor

@RobAndrewHurst The issue with close method should be fixed now.

Lekker!

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.

The fix has put back the modal after closing it.

But if you have multiple modals open from different styles it seems to break.

Screen.Recording.2024-02-01.at.11.29.45.mov

@dbauszus-glx
Copy link
Member Author

@RobAndrewHurst Changing the theme will now remove the closest modal to the legend.

@RobAndrewHurst
Copy link
Contributor

@RobAndrewHurst Changing the theme will now remove the closest modal to the legend.

Very nice! Thanks @dbauszus-glx working well!

Copy link
Contributor

@simon-leech simon-leech left a comment

Choose a reason for hiding this comment

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

@dbauszus-glx
📃 Please can you write this all up in the Config Docs before I can review it in detail, as I'm not quite sure of all the options to test!

❓I applied just this to layer.style "allowModal": true, .
This added the popout modal button but the modal was added with a close button. Is this the expected behaviour and close is by default true?

@dbauszus-glx
Copy link
Member Author

dbauszus-glx commented Feb 1, 2024

@simon-leech The behaviour is still likely to change as we still need to review how legends should work. This PR should only be concerned with the modal element itself.

@simon-leech
Copy link
Contributor

@dbauszus-glx Noticed a little bug.
image

If you add a modal to a style - hit pop out for the modal, then close the modal, then collapse the style - the modal button remains on top of the style element rather than being closed with the style.

Copy link

sonarcloud bot commented Feb 1, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@dbauszus-glx
Copy link
Member Author

The allowModal option has been removed.

Copy link
Contributor

@simon-leech simon-leech left a comment

Choose a reason for hiding this comment

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

@dbauszus-glx Thanks for removing allowModal - much cleaner!

Tested this out and working for me :)

@RobAndrewHurst RobAndrewHurst merged commit a277878 into GEOLYTIX:main Feb 2, 2024
5 checks passed
@dbauszus-glx dbauszus-glx deleted the modal-ui-element branch March 15, 2024 10:48
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants