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

Relation controller popup: fix size, add cssClass and allowDismiss #1044

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

AIC-BV
Copy link
Contributor

@AIC-BV AIC-BV commented Jan 22, 2024

Add the possibility to define the popup size and its custom CSS classes.
You can now define these properties at the manage part of the config_relation.yaml:

myRelation:
    manage:
        size: giant
        cssClass: test

See all popup sizes at https://wintercms.com/docs/v1.2/ui/controls/popup#data-attributes

size already existed, but was hardcoded and not dynamically filled in.
cssClass is added.

@AIC-BV AIC-BV marked this pull request as draft January 22, 2024 15:31
@mjauvin mjauvin added enhancement PRs that implement a new feature or substantial change needs review Issues/PRs that require a review from a maintainer labels Jan 22, 2024
@mjauvin mjauvin self-assigned this Jan 22, 2024
@LukeTowers
Copy link
Member

@AIC-BV @mjauvin what's left on this?

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Feb 21, 2024

@LukeTowers Should be finished

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Feb 22, 2024

Could add allowDismiss (see comments)
#1053

@AIC-BV AIC-BV changed the title Relation controller popup size Relation controller popup: fix size dismiss Feb 23, 2024
@AIC-BV AIC-BV changed the title Relation controller popup: fix size dismiss Relation controller popup: fix size and cssClass, add allowDismiss Feb 23, 2024
@AIC-BV AIC-BV marked this pull request as ready for review February 23, 2024 09:37
@AIC-BV AIC-BV changed the title Relation controller popup: fix size and cssClass, add allowDismiss Relation controller popup: fix size, add cssClass and allowDismiss Feb 23, 2024
@LukeTowers
Copy link
Member

@AIC-BV just a thought, is there a way we could integrate with the change monitor used in the cms section for the tabs to detect when a change has happened inside of a modal and warn / prevent the dismiss behaviour from being triggered in those cases?

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Feb 26, 2024

@AIC-BV just a thought, is there a way we could integrate with the change monitor used in the cms section for the tabs to detect when a change has happened inside of a modal and warn / prevent the dismiss behaviour from being triggered in those cases?

I assume something like this could be a basic for that kind of behaviour

if (this.options.allowDismiss) {
    modal.on('click', function(e) {
        const target = e.target;
        if (target.classList.contains('control-popup')) {

            if (!modal.dataset.hasChanges) {
                modal.hide()
                $('.popup-backdrop').remove()
                $(document.body).removeClass('modal-open')
            } else {
                // popup warning them about unsaved changes
            }

        }
    });

    const inputs = modal.querySelectorAll('input')
    for (const input of inputs) {
        input.addEventListener('change', function() {
            modal.dataset.hasChanges = true
        })
    }
}

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Mar 22, 2024

Thoughts @LukeTowers @mjauvin ?

@LukeTowers
Copy link
Member

@AIC-BV seems fine, have you tested it out?

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Apr 7, 2024

I'm using everything except the latest comment: #1044 (comment)

@LukeTowers
Copy link
Member

@AIC-BV can we remove allowDismiss from this PR for now then? I don't want to support it unless it can be a bit safer first.

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Apr 16, 2024

If its ok for you I will add the popup shortly instead

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Apr 30, 2024

@AIC-BV can we remove allowDismiss from this PR for now then? I don't want to support it unless it can be a bit safer first.

Can you help me getting started?
Don't know how to do this using Winter UI.

Also, will this be enough?
For example: the code below will notice 'changes' when input has been changed, but if it is changed back to its original value, it will still be marked as changed etc. There are some loopholes to this simple logic.

PS:
The PR as is, works perfectly fine. Have been using it a lot, by many people, in production for a month now, but it shows no popup if they have unchanged changes (but it doesn't occur here)

if (this.options.allowDismiss) {
    modal.on('click', function(e) {
        const target = e.target;
        if (target.classList.contains('control-popup')) {

            if (!modal.dataset.hasChanges) {
                modal.hide()
                $('.popup-backdrop').remove()
                $(document.body).removeClass('modal-open')
            } else {
            
                // HOW CAN I DO THIS USING WINTER UI?
                // popup warning them about unsaved changes
                // where they can press 'Cancel' or 'Continue anyways'
            }

        }
    });

    const inputs = modal.querySelectorAll('input')
    for (const input of inputs) {
        input.addEventListener('change', function() {
            modal.dataset.hasChanges = 'true'
        })
    }
}

https://wintercms.com/docs/v1.2/ui/controls/popup#examples

@LukeTowers
Copy link
Member

There's existing code for tracking changes here: https://wintercms.com/docs/v1.2/ui/script/input-monitor

@AIC-BV
Copy link
Contributor Author

AIC-BV commented May 15, 2024

Was making another relation popup and added these properties in the relation_config:
Can't stress enough how good it works and how nice it is.

allowDismiss: true
cssClass: huge-popup

I tried the change monitor and it doesn't show the warning when closing the popup.
The change monitor has this 'issue' as well

For example: the code below will notice 'changes' when input has been changed, but if it is changed back to its original value, it will still be marked as changed

What is left for finishing this PR is a way to show a popup warning the user about unsaved changes using either Input Monitor or custom written JS.

if (this.options.allowDismiss) {
    modal.on('click', function(e) {
        const target = e.target;
        if (target.classList.contains('control-popup')) {

            if (!modal.dataset.hasChanges) {
                modal.hide()
                $('.popup-backdrop').remove()
                $(document.body).removeClass('modal-open')
            } else {
            
                // HOW CAN I DO THIS USING WINTER UI?
                // popup warning them about unsaved changes
                // where they can press 'Cancel' or 'Continue anyways'
                alert('UNSAVED CHANGES!')
               
            }

        }
    });

    const inputs = modal.querySelectorAll('input')
    for (const input of inputs) {
        input.addEventListener('change', function() {
            modal.dataset.hasChanges = 'true'
        })
    }
}

@LukeTowers
Copy link
Member

@jaxwilko any input?

@jaxwilko
Copy link
Member

@jaxwilko any input?

You can use Snowboard.flash() to trigger the flash messaging, I believe it's available in the backend.

Apart from that looks cool, I'll check it out and have a play when I have time :)

AIC-BV added 2 commits August 28, 2024 11:02
Only trigger on mousedown, because when selecting text to copy you sometimes leave the modal and that would close it.
@AIC-BV
Copy link
Contributor Author

AIC-BV commented Aug 28, 2024

Strongly suggest to try it out :)
Works great and is being used here 8 hours/day by 3-4 collegues

@LukeTowers
Copy link
Member

@AIC-BV are you able to record a loom or something showing the new functionality and make a PR to the docs? If you can get those done then I should be able to quickly review and merge this before I push out 1.2.7.

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Aug 30, 2024

  1. This PR fixes the 'size' attribute, which was hardcoded before
  2. Adds the 'cssClass' attribute: allowing you to customise the popup (I make it much bigger because it has important data for production)
  3. And adds support for 'allowDismiss': https://www.loom.com/share/3c10c530854f4e0597f4dd00c632a5cc?sid=08f8142e-55b0-42fc-aae6-fab1b5820c77

allowDismiss = Clicking next to the modal closes it (on mousedown because if you drag to select something and release the mouse button outside the modal, it would close the modal too)
Great for reading the data in the record and not having to scroll down to close it

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Aug 30, 2024

wintercms/docs#207

$(document.body).removeClass('modal-open')
}
});
}
Copy link
Member

Choose a reason for hiding this comment

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

Were you able to get the change monitoring to work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No there is no change monitor.
I remember trying to add it but it was looking at the main record instead of the related record

Copy link
Member

Choose a reason for hiding this comment

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

@jaxwilko is this something you could take a look at? I'm concerned that if we add this functionality without that as a failsafe it could make accidental data loss for users too easy when in theory it shouldn't be too difficult to add the change monitoring before we merge this.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement PRs that implement a new feature or substantial change needs review Issues/PRs that require a review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants