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

Re-position 'highlightElement' on window resize #45

Merged
merged 1 commit into from
Jun 21, 2016
Merged

Re-position 'highlightElement' on window resize #45

merged 1 commit into from
Jun 21, 2016

Conversation

loboulet
Copy link
Contributor

@loboulet loboulet commented Jun 14, 2016

Create a method called setPositionForHighlightElement.

  • This method is called in createHighlightOverlay to set the default position of the highlighted element.
  • We also call this method within a run.debounce on resize of the window to make sure the highlighted item is always at the same place and follows the viewport width.

reposition-highlight-element

Relates to #16

@Boubalou
Copy link

LGTM 👍

@RobbieTheWagner
Copy link
Owner

Thanks @loboulet looks good! Would you mind writing a small test to cover this functionality please? 😃

@loboulet
Copy link
Contributor Author

loboulet commented Jun 15, 2016

How would you like to test this functionality? The only case we can cover is to detect if the method is called on resize right?

@RobbieTheWagner
Copy link
Owner

Yeah, I would probably test that it is called on resize and also test that the css is set correctly and the position of the highlightElement changes.

@RobbieTheWagner
Copy link
Owner

Also, just curious, why do you need the copyStyles functionality @loboulet? I found I only needed it in very rare circumstances, and the normal highlight usually worked, without copying all the styles.

@RobbieTheWagner
Copy link
Owner

@loboulet if you don't have time to write tests, I may be able to help. I'm just pretty swamped with other stuff right now.

@loboulet
Copy link
Contributor Author

loboulet commented Jun 20, 2016

Hey, sorry I was out of town.

Also, just curious, why do you need the copyStyles functionality @loboulet?

I have to highlight icons in the layout and I want them above the overlay

if you don't have time to write tests, I may be able to help.

Yeah I could use some help. I have a lot on my plate right now too

@RobbieTheWagner
Copy link
Owner

@loboulet no worries.

So just using modal and setting styles on the highlighted element with the highlightClass did not work for you? The demo app just uses those options and copyStyles is only needed for cases where that won't work for some reason.

Just wanted to ensure the docs were clear enough on this and you weren't using copyStyles unless you really needed to.

I'll go ahead and merge these PRs and test them myself later then.

@loboulet
Copy link
Contributor Author

Yeah everything seems clear to me.

  • I used copyStyles because I wanted the icon in my complex component to be above the overlay;
  • I could have used the highlightClass, but it was adding the class only on the icon and the possibility to go back to the parent were limited in my case;
  • I also tried to attach the parent and add the highlightClass but it gives me headache working with the CSS to only show what I wanted :\ .

@RobbieTheWagner
Copy link
Owner

Okay, just wanted to make sure it was necessary for your case. Sounds like it is. Thanks again for the PRs!

@RobbieTheWagner RobbieTheWagner merged commit 7fb4e84 into RobbieTheWagner:master Jun 21, 2016
@loboulet
Copy link
Contributor Author

No problem :)

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.

3 participants