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

proposal for get and observe #106

Closed
wants to merge 2 commits into from
Closed

Conversation

alisman
Copy link
Collaborator

@alisman alisman commented Dec 9, 2016

What? Why?

Components need to be able to fetch data and then observe change to that same data. Rather than put the data in redux store, we allow components to subscribe to the QuerySession object.

Changes proposed in this pull request:

  • a
  • b

Checks

  • 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:

@adamabeshouse
Copy link
Contributor

Whereas this way, there has to be observer registration in any component that wants to use data, we could instead have only one object which observes, and keeps the data in the store fresh, and then use React and mapStateToProps to implement the observer pattern in components.

Why do you prefer the proposed approach?

@adufilie
Copy link
Contributor

adufilie commented Dec 9, 2016

I pushed some changes, adding missing parts

@adufilie
Copy link
Contributor

adufilie commented Dec 9, 2016

I've already solved this messy code issue in the past:
https://github.com/adufilie/WeaveJS/blob/namespaces/srcts/weavejs/util/ReactUtils.tsx#L339

@jjgao jjgao temporarily deployed to cbioportal-frontend-pr-106 December 9, 2016 17:15 Inactive
@alisman alisman closed this Jan 25, 2017
onursumer added a commit to onursumer/cbioportal-frontend that referenced this pull request Dec 18, 2019
Added some margin to the right aligned columns
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 11, 2022
Column labels, and allow set initial cell width
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 16, 2022
Column labels, and allow set initial cell width
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 18, 2022
Column labels, and allow set initial cell width
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 19, 2022
Column labels, and allow set initial cell width
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 20, 2022
Column labels, and allow set initial cell width
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 23, 2022
Column labels, and allow set initial cell width
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 24, 2022
Column labels, and allow set initial cell width
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 25, 2022
Column labels, and allow set initial cell width
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 26, 2022
Column labels, and allow set initial cell width
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 27, 2022
Column labels, and allow set initial cell width
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 27, 2022
Column labels, and allow set initial cell width
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 27, 2022
Column labels, and allow set initial cell width
alisman pushed a commit to alisman/cbioportal-frontend that referenced this pull request Jun 2, 2022
Column labels, and allow set initial cell width
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request Jun 6, 2022
Column labels, and allow set initial cell width
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request Jun 6, 2022
Column labels, and allow set initial cell width
alisman pushed a commit to onursumer/cbioportal-frontend that referenced this pull request Jun 8, 2022
Column labels, and allow set initial cell width
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants