Refactor and fix on scroll #41
Conversation
There was a problem hiding this comment.
Hi @caillou,
Thank you for your contribution! The floater repositioning bug was unknown to us, and a lot of the refactoring changes are ones I also would have liked to make eventually or are welcome surprises... so I have been reading this PR with a lot of joy :)
There were 2 things I felt a bit iffy about (the verbose deconstruction of the target element style and the whole isTabOrArrow self-documenting function which feels a tad bit much for just that one use), but honestly I think there is also something to be said for these changes, so I'm fine keeping them in.
There is a merge conflict which needs to be solved, and I added a couple minor naming inconsistency comments in the test file. So if you can fix those points, we can go ahead and merge this!
(By the way, I have now set my account to watch this repository, so you can expect a quicker response than two months from now on :) )
| }); | ||
| }); | ||
| }); | ||
|
|
||
| function getTargetAndContainer () { |
There was a problem hiding this comment.
I think using the word create is more descriptive:
| function getTargetAndContainer () { | |
| function createTargetAndContainer() { |
| }); | ||
|
|
||
| it('Should create the \'floater\' element when it is not present yet', () => { | ||
| it(`Should not recreate the 'floater' element when it's already present created`, () => { |
There was a problem hiding this comment.
| it(`Should not recreate the 'floater' element when it's already present created`, () => { | |
| it(`Should not recreate the 'floater' element when it's already present`, () => { |
There was a problem hiding this comment.
Oh I just realised this was also in the original test... oh well haha.
|
|
||
|
|
| @@ -88,22 +85,72 @@ describe('Floating focus', () => { | |||
| expect(floatingFocus.disableFloatingFocus).toHaveBeenCalled(); | |||
| }); | |||
|
|
|||
| it('Should only reposition if a target and \'floater\' was set, when scrolling or resizing', async () => { | |||
| it('Scrolling should not reposition by default.', async () => { | |||
There was a problem hiding this comment.
I'm always a bit pedantic about the name of tests with it hehe... Usually I'd actually prefer something like it('does not reposition the floater by default' (so without "should") but let's stick to the convention of the rest for the tests.
| it('Scrolling should not reposition by default.', async () => { | |
| it('Should not reposition the floater by default', async () => { |
| expect(floatingFocus.repositionElement).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('Scrolling should not reposition only when focusing.', async () => { |
There was a problem hiding this comment.
| it('Scrolling should not reposition only when focusing.', async () => { | |
| it('Should not reposition the floater when focusing but floating focus is inactive', async () => { |
|
|
||
| floatingFocus.floater = document.createElement('div'); | ||
| floatingFocus.target = document.createElement('div'); | ||
| it('Scrolling should reposition when focusing if floating is shown.', async () => { |
There was a problem hiding this comment.
| it('Scrolling should reposition when focusing if floating is shown.', async () => { | |
| it('Should reposition the floater when focusing and floating focus is active', async () => { |
|
|
||
|
|
||
| it('Scrolling should not reposition after blur.', async () => { |
There was a problem hiding this comment.
| it('Scrolling should not reposition after blur.', async () => { | |
| it('Should not reposition the floater after blur', async () => { |
|
|
||
| it('Scrolling should not reposition after click.', async () => { |
There was a problem hiding this comment.
| it('Scrolling should not reposition after click.', async () => { | |
| it('Should not reposition the floater after click', async () => { |
| this.floater.classList.add('visible'); | ||
| this.floater.classList.add('helper'); | ||
| this.floater.classList.add('moving'); | ||
| this.floater.classList.add('visible', 'helper', 'moving'); |
There was a problem hiding this comment.
This breaks in all versions of IE.
| const targetStyle = window.getComputedStyle(target); | ||
| const padding = targetStyle.outlineOffset || null; | ||
| const { | ||
| outlineOffset, |
There was a problem hiding this comment.
Does this really make the code more readable?
In an effort to review this library before usage, I started to refactor parts of it. I ended up going a bit over-board with this. I hope this is ok:
node-sasswithsass.While some of these changes are very much opinion-based, I would still recommend fixing the
onScrollbug and going tosass.WDYT?