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

remove scriptaculous/effects.js dependency in js/prototype/validation.js #2609

Merged
merged 1 commit into from
Jan 12, 2023
Merged

remove scriptaculous/effects.js dependency in js/prototype/validation.js #2609

merged 1 commit into from
Jan 12, 2023

Conversation

empiricompany
Copy link
Contributor

Description (*)

if scriptaculous/effects.js is not loaded hideAdvice function return an error
make validation.js indipendent from scriptaculous/effects.js so we can remove this library to optimize page load
PS: like in showAdvice() function

Manual testing scenarios (*)

  1. remove scriptaculous/effects.js
<layout version="0.1.0">
	<default>
		<reference name="head">
			<action method="removeItem">
				<type>js</type>
				<name>scriptaculous/effects.js</name>
			</action>
		</reference>
	</default>
</layout>
  1. test some form validation, first input wrong data then enter validated data
    error advice not disappear and return error:
    Uncaught ReferenceError: Effect is not defined

remove scriptaculous/effects.js dependency
@github-actions github-actions bot added the JavaScript Relates to js/* label Sep 18, 2022
@fballiano
Copy link
Contributor

good idea! what about we try to implement the fade effect in css only and we remove to call to Effect.fade completely?

@empiricompany
Copy link
Contributor Author

good idea! what about we try to implement the fade effect in css only and we remove to call to Effect.fade completely?

what about to implement fade effect with WAAPI
https://developer.mozilla.org/en-US/docs/Web/API/Web_Animations_API/Web_Animations_API_Concepts

seems its widly compatibly
https://caniuse.com/web-animation

i have a ready solution:


showAdvice : function(elm, advice, adviceName){
        if(!elm.advices){
            elm.advices = new Hash();
        }
        else{
            elm.advices.each(function(pair){
                if (!advice || pair.value.id != advice.id) {
                    // hide non-current advice after delay
                    this.hideAdvice(elm, pair.value);
                }
            }.bind(this));
        }
        elm.advices.set(adviceName, advice);
        if(typeof Effect == 'undefined') {
            advice.style.display = 'block';
            advice.animate([
                { opacity: '0' },
                { opacity: '0.5', offset: 0.5 },
                { opacity: '1', offset: 1 }
            ], 
            {
                duration: 800,
                easing: 'linear', 
                iterations: 1
            }).onfinish = () => advice.show();
        } else {
            if(!advice._adviceAbsolutize) {
                new Effect.Appear(advice, {duration : 1 });
            } else {
                Position.absolutize(advice);
                advice.show();
                advice.setStyle({
                    'top':advice._adviceTop,
                    'left': advice._adviceLeft,
                    'width': advice._adviceWidth,
                    'z-index': 1000
                });
                advice.addClassName('advice-absolute');
            }
        }
    },
    hideAdvice : function(elm, advice){
        if (advice != null) {
            if(typeof Effect == 'undefined') {
                advice.animate([
                    { opacity: '1' },
                    { opacity: '0', offset: 0.5 },
                    { opacity: '0', offset: 1 }
                ], 
                {
                    duration: 800,
                    easing: 'linear', 
                    iterations: 1
                }).onfinish = () => advice.hide();
            } else {
                new Effect.Fade(advice, {duration : 1, afterFinishInternal : function() {advice.hide();}});
            }
        }
    },

@sreichel
Copy link
Contributor

Nice :) Tested.

But we cannot remove effects.js now ... in admin config youll see this Uncaught controls.js requires including script.aculo.us' effects.js library

@sreichel
Copy link
Contributor

Why closed?

@empiricompany
Copy link
Contributor Author

sorry I accidentally closed this PR

@sreichel yes its purpose is only giving a replacement for Effect.fade*() in a custom theme on frontend that not include scriptaculous/effects.js to optimize js size and the page load

@empiricompany empiricompany reopened this Sep 18, 2022
@sreichel
Copy link
Contributor

good idea! what about we try to implement the fade effect in css only and we remove to call to Effect.fade completely?

What do you think about removing fallback to effects.js?

@empiricompany empiricompany closed this by deleting the head repository Sep 26, 2022
@empiricompany empiricompany reopened this Sep 26, 2022
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

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

I'm approving this since we can't remove scriptaculous at the moment, also the showAdvice() has a double implementation, using effects or not, so i guess it's correct to have the same for hideAdvice()

@fballiano fballiano merged commit 7f530f7 into OpenMage:1.9.4.x Jan 12, 2023
fballiano pushed a commit that referenced this pull request Jan 12, 2023
@fballiano
Copy link
Contributor

cherry-picked to v20 since no conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JavaScript Relates to js/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants