VAF chart changes and refactoring

[forus]

What? Why?

Improve VAF chat code and add 2 small UI improvements.

Changes proposed in this pull request:

• Add ellipsis when the sample name is too long. Hover over will show the full sample name
• Merge several samples in a single circle by x coordinate. So sample 1, 2, 3, 4, and 5 will make a circle with "1-5" label
• Refactor VAFChart. It does not update the state for the parent component anymore (the wrapper). Many calculations were moved up to the wrapper.

Checks

• Is this PR adding logic based on one or more clinical attributes? If yes, please make sure validation for this attribute is also present in the data validation / data loading layers (in backend repo) and documented in File-Formats Clinical data section!
• Follows 7 rules of great commit messages. For most PRs a single commit should suffice, in some cases multiple topical commits can be useful. During review it is ok to see tiny commits (e.g. Fix reviewer comments), but right before the code gets merged to master or rc branch, any such commits should be squashed since they are useless to the other developers. Definitely avoid merge commits, use rebase instead.
• Follows the Airbnb React/JSX Style guide.
• Make sure your commit messages end with a Signed-off-by string (this line
  can be automatically added by git if you run the git-commit command with
  the -s option)

Any screenshots or GIFs?

If this is a new visual feature please add a before/after screenshot or gif
here with e.g. GifGrabber.

Notify reviewers

Read our Pull request merging
policy. If you are part of the
cBioPortal organization, notify the approprate team (remove inappropriate):

@cBioPortal/frontend

If you are not part of the cBioPortal organization look at who worked on the
file before you. Please use git blame <filename> to determine that
and notify them here:

[alisman]

super minor but i like to use reduce for this kind of operation so that you get direct assignment, or even just direct return. Matters more in longer routines. You don't need to fix this.

const sampleIds = this.sampleEvents.reduce()

[alisman]

did you mean to take @observable.ref off?

[forus]

Yes, although I am not sure whether it's needed.

The documentation says:

@observable.ref - Decorator that creates an observable that only observes the references, but doesn't try to turn the assigned value into an observable.ts.

We reassign mouseOverMutation from the setter. Won't the field stop to be observable after reassignment then?

[alisman]

should be computed

[forus]

I guess I know why: Because you need to trigger recalculation when some of the variables groupColor rely on change.
I am wondering when one would not choose to make something @computed? When it's a pure function that only depends on it's input?

[alisman]

should be computed. autobind decorator is only for methods used as callbacks. autobind ensures that bound context ("this") is the instance of the class.

[alisman]

no autobind

[alisman]

my fault. lets kill this.

[alisman]

lets call this property "timelineStore"

[alisman]

bunch of places where @autoBind is used incorrectly. i think this was from earlier round of coding, not yours ruslan, but let fix it nonetheless. should only use @autoBind on a method which is used in a DOM callback, eg:

handleClick --> <a onClick={handleClick}>

[forus]

bunch of places where @autoBind is used incorrectly. i think this was from earlier round of coding, not yours ruslan, but let fix it nonetheless. should only use @autoBind on a method which is used in a DOM callback, eg:

handleClick --> <a onClick={handleClick}>

It seems like we use arrow functions everywhere. e.g. onMutationClick={m => this.props.dataStore.toggleSelectedMutation(m)}. Hence we don't need @autobind anymore?

[forus]

@alisman I've pushed the changes. Please have another look