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

Fixed modal window responsiveness #7534

Closed
wants to merge 3 commits into from
Closed

Fixed modal window responsiveness #7534

wants to merge 3 commits into from

Conversation

@karser
Copy link

karser commented May 16, 2019

Please be sure you are submitting this against the staging branch.

Q A
Bug fix? yes
New feature?
MauticBox URL https://mautibox.com/7534/page/preview/4
Related user documentation PR URL
Related developer documentation PR URL
Issues addressed (#s or URLs)
BC breaks?
Deprecations?

Description:

Discord conversation:
image

Before:
image

After:
image

Steps to reproduce the bug:

  1. Put a form snippet on a Landing Page, e.g {form=2}
  2. Add a call-to-action button which opens the form in a modal window. See the docs
<a class="btn btn-default" href="#" id="call-to-action-one">Click me</a> <!-- trigger button -->
{mauticform id=2 style=modal element=#call-to-action-one} <!-- setup modal form -->
  1. Emulate mobile screen using Chrome Dev Tools and click the button.
@npracht npracht added this to the 2.16.0 milestone May 16, 2019
@karser

This comment has been minimized.

Copy link
Author

karser commented May 17, 2019

Hello @npracht, is there a chance to see this PR merged in 2.15.x? It doesn't break BC, it's a bug-fix, isn't it?

@npracht

This comment has been minimized.

Copy link
Member

npracht commented May 17, 2019

@kuzmany

This comment has been minimized.

Copy link
Contributor

kuzmany commented May 17, 2019

First, remove media/css/modal.min.css from pull requests (add from staging).
With new release we will regenrate minified css and js.

If you will make two tests today, I can add it to 2.15.2.

@karser karser force-pushed the karser:modal-fix branch May 17, 2019
@karser

This comment has been minimized.

Copy link
Author

karser commented May 17, 2019

I tested this PR on Mautibox. https://mautibox.com/7534/s/pages/edit/4

I discovered 2 issues:

  1. The file media/css/modal.min.css is not updated.
    That is, neither app/console mautic:assets:generate nor grunt less updates media/css/modal.min.css. Speaking of which, is there a reason the minified asset is under VCS?

In this PR I generated it using

npm install uglifycss -g
uglifycss media/css/modal.css > media/css/modal.min.css

I will revert and commit modal.min.css.

  1. Due to the different Mauticbox webserver configuration I encounter the insecure https => http redirect error when trying to open a form in the iframe.

It happens in mautic-form-src.js:

        Form.getFormLink = function(options) {
            var index = (Core.devMode()) ? 'index_dev.php' : 'index.php';
            return Core.getMauticBaseUrl() + index + '/form/' + options.data['id'] + '?' + options.params;
        };

Here is the behavior on mauticbox:

https://mautibox.com/7534/index.php/form/2 [301] => https://mautibox.com/7534/form/2 [200]
https://mautibox.com/7534/index_dev.php/form/2 [200]

So I'll remove index.php from getFormLink function since index.php is redundant.

@karser karser force-pushed the karser:modal-fix branch 2 times, most recently May 17, 2019
@karser

This comment has been minimized.

Copy link
Author

karser commented May 17, 2019

Done, everything works as expected: https://mautibox.com/7534/page/preview/4
Click on the link and see if modal is responsive on mobile.
cc @kuzmany @npracht

@kuzmany

This comment has been minimized.

Copy link
Contributor

kuzmany commented May 17, 2019

media/css/modal.min.css it's not reverted
Mautibox should regenerate assets on every commit is pushed.

@karser

This comment has been minimized.

Copy link
Author

karser commented May 17, 2019

media/css/modal.min.css it's not reverted

After you first comment I removed the file and I was surprised that modal.min.css wasn't generated by Mauticbox.
Then I reverted it, meaning committed back generated file, see my previous comment

@karser

This comment has been minimized.

Copy link
Author

karser commented May 17, 2019

What does Mauticbox launches to generate assets?
Neither app/console mautic:assets:generate nor grunt less updates media/css/modal.min.css

@kuzmany

This comment has been minimized.

Copy link
Contributor

kuzmany commented May 17, 2019

@karser You're right. Then it's okay.

Release 2.15.2
@npracht npracht added this to Ready to Test (first time) in Mautic 2 Aug 15, 2019
Woeler and others added 2 commits Oct 8, 2019
Release 2.15.3
@karser karser force-pushed the karser:modal-fix branch to 8e57823 Dec 5, 2019
@npracht npracht modified the milestone: 2.16.0 Jan 23, 2020
@RCheesley RCheesley added this to the 2.16.1 milestone Mar 9, 2020
@npracht npracht added this to Ready to test in Mautic 2 Mar 10, 2020
@RCheesley RCheesley moved this from Ready to test to Needs a second test/review in Mautic 2 Mar 17, 2020
@RCheesley

This comment has been minimized.

Copy link
Member

RCheesley commented Mar 18, 2020

Similar to #7510 I am unable to reproduce this. I can't get it to show the form in a modal. Perhaps you can provide some guidance @karser ?

@dennisameling

This comment has been minimized.

Copy link
Member

dennisameling commented Mar 19, 2020

Also tested on a production instance and cannot get this to work in 2.16.0:

image

Looks like {mauticform} isn't replaced, could it be that it was renamed to {form} instead? With {form} it seems to ignore the style=modal and show the form immediately instead. Something might have changed after this PR was created in May 2019. :(

@dennisameling dennisameling moved this from Needs a second test/review to Backlog in Mautic 2 Mar 19, 2020
@RCheesley

This comment has been minimized.

Copy link
Member

RCheesley commented Mar 19, 2020

I tend to agree, I think therefore we should close the PR - if it can be refactored at a later date we can re-open and consider then.

@RCheesley RCheesley closed this Mar 19, 2020
Mautic 2 automation moved this from Backlog to Merged Mar 19, 2020
@mautibot

This comment has been minimized.

Copy link

mautibot commented Mar 20, 2020

This pull request has been mentioned on Mautic Community Forums. There might be relevant details there:

https://forum.mautic.org/t/announcing-mautic-2-16-1-beta/13438/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Mautic 2
  
Merged
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.