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

[Transition] Remember inline style display and dont show hidden objects like <script> #357

Merged

Conversation

lubber-de
Copy link
Member

@lubber-de lubber-de commented Jan 3, 2019

Description

Transition did always detect "block" as display value and did not respect or remembered an initial given inline block style which was probably wanted by the user. For example inline or even none, if an element within a transition should definately not be shown.
Not only because of this, but it was also displaying script or other unwanted tags 🙄
script, link or style tags are now skipped all the time.

Especially for accordion it turns out to be usefull that transition should be skipped if the item has initially been set to be hidden. This is true in most cases for the to be animated element itself, but in an accordion there could be some elements within the container where those elements should be kept hidden.

Therefore i added a new setting

skipInlineHidden: false ; // (default false to stay backward compatible) 

to the transition module.
The accordion module uses this now to still hide any child element of a accordion container (not the container itself!) in case a child element was already initially hidden.
See the source of the fiddle below for better explanation 😉

For additional testing i injected the new transition.js into all of my local gh-pages-branch to make sure no other components had negative side effects (way too many examples otherwise to put into a fiddle)

If you also want to test it, you can simply:

<script src="/dist/semantic.min.js"></script>
<script src="/dist/transition.js"></script>

In the following docs html files: accordion, popup,dimmer,transition,form, search, modal, toast, visibility,modal,dropdown (those modules make use of transition.js , thats why. The docs itself use transition.js on every page)

Testcase

Broken

http://jsfiddle.net/qLp3md9n/

Fixed

http://jsfiddle.net/qLp3md9n/2/

Closes

Semantic-Org/Semantic-UI#2988
Semantic-Org/Semantic-UI#3171
Semantic-Org/Semantic-UI#6735

@lubber-de lubber-de added type/bug Any issue which is a bug or PR which fixes a bug lang/javascript Anything involving JavaScript state/awaiting-reviews Pull requests which are waiting for reviews tag/sui-issue Taken from an existing Issue/PR of SUI labels Jan 3, 2019
@lubber-de
Copy link
Member Author

Just found out i had to make an explicit check for script, link or style tags on mobile. So in case the above fiddle does still show the JS Code, either wait until jsdelivr has renewed its cache or put the whole transition.js in the fiddle before running it...

@lubber-de lubber-de added this to the 2.7.x milestone Jan 3, 2019
@lubber-de
Copy link
Member Author

Sorry, It seems I broke it for the docs.. I put this on hold until I fixed that also...

@lubber-de lubber-de added state/on-hold Issues and pull requests which are on hold for any reason and removed state/awaiting-reviews Pull requests which are waiting for reviews labels Jan 3, 2019
… option, so it does not break other components and only accordion needs this so far,
@lubber-de lubber-de added state/awaiting-reviews Pull requests which are waiting for reviews state/awaiting-docs Pull requests which need doc changes/additions and removed state/on-hold Issues and pull requests which are on hold for any reason labels Jan 3, 2019
@lubber-de
Copy link
Member Author

Fixed it 💦 ...
Updated the fiddle link and description.
Ready for review 😄

@lubber-de lubber-de added state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo and removed state/awaiting-docs Pull requests which need doc changes/additions labels Jan 4, 2019
Copy link
Member

@y0hami y0hami left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@ColinFrick ColinFrick left a comment

Choose a reason for hiding this comment

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

LGTM

@y0hami y0hami merged commit 73cd274 into fomantic:develop Jan 22, 2019
@lubber-de lubber-de modified the milestones: 2.7.x, 2.7.2 Jan 22, 2019
@lubber-de lubber-de removed the state/awaiting-reviews Pull requests which are waiting for reviews label Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/javascript Anything involving JavaScript state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo tag/sui-issue Taken from an existing Issue/PR of SUI type/bug Any issue which is a bug or PR which fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants