Skip to content

Conversation

@guillaumevincent
Copy link
Contributor

@guillaumevincent guillaumevincent commented Oct 25, 2018

To make the code more consistent, I added a method getUniqueId that is used by all components that
need a unique id. I put this id in the defaultProps object. Add also some tests for util
file.

@patternfly-build
Copy link
Collaborator

PatternFly-React preview: https://830-pr-patternfly-react-patternfly.surge.sh

@coveralls
Copy link

coveralls commented Oct 25, 2018

Pull Request Test Coverage Report for Build 2866

  • 5 of 5 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 81.262%

Totals Coverage Status
Change from base Build 2861: -0.01%
Covered Lines: 3463
Relevant Lines: 4018

💛 - Coveralls

};

class Progress extends Component {
constructor(props) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great PR! But we do need to initialize the unique id in the constructors. Or at least have some other way of assigning unique id to the component. Props get evaluated before the component instances are created so if you have multiple of the same components on the page they will all have the same id otherwise.

Math.random()
.toString(36)
.slice(2),
id: getUniqueId(),
Copy link
Contributor

Choose a reason for hiding this comment

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

this does not work. Default props are only instantiated once on when this variable , defaultProps is created. The id will need to be setup in a "constructor" or better as a class property

class Something extends React.Component {
  this.id = this.props.id || getUniqueId();
}

return input[0].toUpperCase() + input.substring(1);
}

export function getUniqueId() {
Copy link
Contributor

Choose a reason for hiding this comment

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

might be nice to allow a prefix for this so that we can make the id's somewhat useful

getUniqueId(prefix = '') {
  return (
   `${prefix}-` + new Date().getTime() +
    Math.random()
      .toString(36)
      .slice(2)
  );
}

this would allow us to do something like getUniqueId('pf-switch') we could even add the pf- by default and have the preson just pass the component name to it.

@guillaumevincent
Copy link
Contributor Author

@dmiller9911 and @jschuler done
Tell me if this implementation is good

@guillaumevincent guillaumevincent force-pushed the unique_id branch 2 times, most recently from 03b2e06 to 1b3ef68 Compare October 25, 2018 17:32
jschuler
jschuler previously approved these changes Oct 25, 2018
dmiller9911
dmiller9911 previously approved these changes Oct 26, 2018
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

Looks good! Could you just merge the merge conflicts. Thanks!

To make the code more consistent, I added a method getUniqueId that is used by all components that
need a unique id. I put this id recovery in the defaultProps object. Add also some tests for util
file.
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

LGtM

@tlabaj tlabaj merged commit b8bc4b1 into patternfly:master Oct 26, 2018
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.

8 participants