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

docs: Affix - don't include arrow-functions inside component props; use refs instead #4037

Closed
jkoudys opened this issue Nov 28, 2016 · 3 comments
Labels
help wanted The suggestion or request has been accepted, we need you to help us by sending a pull request.

Comments

@jkoudys
Copy link

jkoudys commented Nov 28, 2016

In /components/affix doc, I see this bad practice:

const Demo = () => {
  return (
    <div className="view-port">
      <div id="scrollable-container">
        <div className="background">
          <br />
          <br />
          <br />
          <Affix target={() => document.getElementById('scrollable-container')} offsetTop={20}>
            <Button type="primary">Fixed at the top of container</Button>
          </Affix>
        </div>
      </div>
    </div>
  );
};

What's wrong with this:

  1. Using an arrow-function with a direct return. () => { return ( is pointless noise. Say () => ( instead.
  2. You're using a getElementById to directly reference an element rendered in the same component. Huge bad-practice. This is what refs are for: https://facebook.github.io/react/docs/refs-and-the-dom.html
  3. Don't declare arrow-functions inside a component prop like I see in <Affix target. This was popular a year ago, but is considered bad-practice now, and violates many eslint rules. Using a ref properly will fix this too.
@jkoudys
Copy link
Author

jkoudys commented Nov 28, 2016

As a side-note, I understand it's just a demo, but seeing <br /><br /><br /> is an ugly way to style. If it's meant to demo some components taking vertical space above the <Affix>, maybe a <p> with some content would make more sense. I really don't want to work with developers who learn from this example, and think that using multiple <br />s is a wise way to vertically align content.

@benjycui
Copy link
Contributor

Yep, you are right... If you have any idea to improve documentation and demos, just create a PR to master.

@benjycui benjycui added the help wanted The suggestion or request has been accepted, we need you to help us by sending a pull request. label Nov 29, 2016
@afc163 afc163 closed this as completed in c7c886f Dec 3, 2016
@lock
Copy link

lock bot commented May 3, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted The suggestion or request has been accepted, we need you to help us by sending a pull request.
Projects
None yet
Development

No branches or pull requests

2 participants