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

Improve detection strategy of flip modifier #82

Merged
merged 8 commits into from
Sep 23, 2016

Conversation

adevnadia
Copy link

This PR is an attempt to solve the issue #78. There are two commits here.

The first one is solving "moving after target" problem. I.e. popper will behave like it doesn't have 'preventOverflow' modifier, but still will flip itself from the parent. It looks like this:

https://goo.gl/photos/H5QafmbZ1xjVuHeD6

Enabling this behaviour would look like this:

modifiers: {
    preventOverflow: {
        moveWithTarget: true
    }
}

The second commit is improving flipping behavior so that popper can also flip its position's variation ('start' to 'end' and vice versa). It looks like this:

https://goo.gl/photos/MtneBBm9nNd6Nynz6

modifiers: {
    flip: {
        flipVariations: true
    }
}

@FezVrasta
Copy link
Member

FezVrasta commented Sep 4, 2016

It looks cool, thanks!

The build has some problem with the authentication... No idea what changed, I'll take a look.

I have a flight for SF in an hour so I'll not be able to review this PR anytime soon, I'll work on it once I'm arrived. For now I can only give you feedbacks about the code quality.

Could you add some functional test to test the new behaviors you added?

@@ -14,6 +15,7 @@ import runModifiers from '../utils/runModifiers';
* @argument {Object} options - Modifiers configuration and options
* @returns {Object} The data object, properly modified
*/

Copy link
Member

Choose a reason for hiding this comment

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

No space between JSdocs comments and the function please 🙃

@adevnadia
Copy link
Author

Sorry about the formatting, too used to rely on linting for this kind of things :) I'll fix it tomorrow. Have a safe flight!

@adevnadia
Copy link
Author

@FezVrasta updated according to your comments, sorry for the delay

@FezVrasta
Copy link
Member

Hey thanks! I'm still trying to figure out how to make the tests pass also on PRs, I'm going to try some suggestions from Sauce Labs in the next days. I'm sorry but I'm not very responsive these days due to the trip to SF, once I'll be back at home I'll be responsive as usual!

@FezVrasta
Copy link
Member

@nadiam84 I sent a PR to this PR (LOL) to make the tests pass here.

I'd say that once you merge it and some tests for this new behavior are added we can merge this into v1-dev!

In the next days I'm going to finish all the remaining work on v1 and I'll release the first beta

@FezVrasta
Copy link
Member

I was playing with the flipWithoutPreventOverflow.html visual test and it doesn't seem to work...

flip

As you see, it follows the ref only on vertical scroll, on horizontal it stays between boundaries.

@adevnadia
Copy link
Author

@FezVrasta did you build the js files before this? I haven't commited any build results as per your contribution guidlines :)
I'll write some tests today/tomorrow, completely forgot about them. Although the saucelabs tests are still not working.

@FezVrasta
Copy link
Member

I think I compiled them... But now you make me doubt about it lol.

I'll check again shortly

@FezVrasta
Copy link
Member

Ah! You were right lol. Yes it seems to work 🙃

Need some tests to test the new functionalities, it's okay that the old tests still work but we must be sure that the new features are tested as well.

@adevnadia
Copy link
Author

@FezVrasta updated the PR with tests. Locally they were green :D Checked in the latest Chrome, Firefox and Safari.

@adevnadia
Copy link
Author

@FezVrasta gentle nudge

@FezVrasta
Copy link
Member

Yes sorry! Forgot about it. The tests are failing because of a problem with Sauce Labs... Going to merge this anyway.

@FezVrasta FezVrasta merged commit 14e635a into floating-ui:v1-dev Sep 23, 2016
@FezVrasta FezVrasta mentioned this pull request Sep 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants