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

SRC files synced - now build process create proper output #234

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

SRC files synced - now build process create proper output #234

wants to merge 3 commits into from

Conversation

svobik7
Copy link
Contributor

@svobik7 svobik7 commented May 6, 2019

With new horizontal lazy load support I noticed that src files were not synced with current output files.

I made some updates to source files so that yarn build and yarn test can be used and won't break current output.

I also run test and all passed but one. I investigated a bit on why this single test failed and noticed that there were some changes in past which causing this test to fail.

@ameerthehacker could you please check if mentioned test is ok and should pass or is no longer valid? Before you merge this PR. Thank you

LazyLoad scrollContainer should focus on a different scroll con
tainer FAILED

describe('Overflow', () => {
    // https://github.com/jasonslyvia/react-lazyload/issues/71
    // http://stackoverflow.com/a/6433475/761124
    it('should not detect a overflow container when only one of the scroll property is auto\/scroll', () => {
      ReactDOM.render(
        <div
          id="realOverflowContainer"
          style={{ height: '600px', overflow: 'auto' }}
        >
          <div
            id="fakeOverflowContainer"
            style={{ height: '300px', overflowX: 'hidden' }}
            className="container"
          >
            <LazyLoad height={200} overflow><div style={{ height: '200px' }} className="something">123</div></LazyLoad>
            <LazyLoad height={200} overflow><div style={{ height: '200px' }} className="something">123</div></LazyLoad>
            <LazyLoad height={200} overflow><div style={{ height: '200px' }} className="treasure">123</div></LazyLoad>
          </div>
        </div>
      , div);

      const container = document.querySelector('.container');
      expect(container.querySelector('.something')).to.exist;
      expect(container.querySelector('.lazyload-placeholder')).not.to.exist;
      expect(container.querySelector('.treasure')).to.exist;
    });
  });

Screenshot 2019-05-06 at 14 47 16

@svobik7
Copy link
Contributor Author

svobik7 commented May 6, 2019

@ameerthehacker sorry about c685166 - I accidentally push this commit to master so than I had to revert it back. It contains another feature and I will publish PR soon after this PR.

@ameerthehacker
Copy link
Collaborator

ameerthehacker commented May 7, 2019

@svobik7 does this PR fix it? and also can you please fix things that are making the test to fail?

@svobik7
Copy link
Contributor Author

svobik7 commented May 7, 2019

@ameerthehacker I think we do not understand each other :-)

As there were some direct changes in lib folder before (without chaging src files) I am unable to decide on my own if this failing test is any more valid or not.

See current inconsistency in master:

Current output lib

if (overflowRegex.test(overflow) || overflowRegex.test(overflowX) || overflowRegex.test(overflowY)) {

Current src

if (overflowRegex.test(overflow) && overflowRegex.test(overflowX) && overflowRegex.test(overflowY)) {

It is easy to fix it so the test will pass but then the output lib code will be different then current release.

I think this inconsistency happend during #167

So should we change or omit that failing test?

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