Push pagination control to CustomTable #31

yebrahim · 2018-11-03T06:20:18Z

Since the logic to manage pagination (page token, size, sort order and column) is repeated in every user component of CustomTable, this reduces overhead for maintenance and testing significantly.

This change is

rileyjbauer

This is great! A lot of simplification

Reviewed 4 of 10 files at r1, 7 of 7 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yebrahim, @gaoning777, and @jlewi)

frontend/src/components/CustomTable.tsx, line 177 at r2 (raw file):

      maxPageIndex: Number.MAX_SAFE_INTEGER,
      pageSize: 10,
      sortBy: props.initialSortColumn || props.columns.length ? props.columns[0].sortKey || '' : '',

Can you add parentheses around the second expression?

frontend/src/components/CustomTable.tsx, line 373 at r2 (raw file):

    if (sortBy) {
      const sortOrder = this.state.sortBy === sortBy ?
        (this.state.sortOrder === 'asc' ? 'desc' : 'asc') : 'asc';

this logic is a bit dense, but it's just flipping the sort direction, right? can you add a small comment indicating such?

yebrahim · 2018-11-03T21:37:18Z

/test mlpipeline-presubmit-e2e-test

yebrahim

Reviewable status: 10 of 11 files reviewed, 2 unresolved discussions (waiting on @rileyjbauer, @gaoning777, and @jlewi)

frontend/src/components/CustomTable.tsx, line 177 at r2 (raw file):

Previously, rileyjbauer (Riley Bauer) wrote…

Can you add parentheses around the second expression?

Done.

frontend/src/components/CustomTable.tsx, line 373 at r2 (raw file):

Previously, rileyjbauer (Riley Bauer) wrote…

this logic is a bit dense, but it's just flipping the sort direction, right? can you add a small comment indicating such?

Done.

rileyjbauer

Reviewable status: 10 of 11 files reviewed, 2 unresolved discussions (waiting on @rileyjbauer, @gaoning777, and @jlewi)

rileyjbauer · 2018-11-04T02:52:14Z

/lgtm

rileyjbauer · 2018-11-04T02:52:46Z

/approve

k8s-ci-robot · 2018-11-04T02:52:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rileyjbauer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rileyjbauer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

yebrahim · 2018-11-04T02:58:40Z

I think you need to
/lgtm
Again

k8s-ci-robot · 2018-11-04T02:58:40Z

@yebrahim: you cannot LGTM your own PR.

In response to this:

I think you need to
/lgtm
Again

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

google-prow-robot · 2018-11-04T02:58:40Z

@yebrahim: you cannot LGTM your own PR.

In response to this:

I think you need to
/lgtm
Again

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rileyjbauer · 2018-11-04T07:04:49Z

/lgtm

…ing (kubeflow#31) * We were checking the wrong variable in the loop. * This was a typo introduced when we switched to supporting multiple workflows. * Fix kubeflow#30

* Change e2e test job to point to kserve repo * Fix prow config * Remove dir dep * fix image-transformer built image tag (kubeflow#44) * fix image-transformer built image tag Signed-off-by: Theofilos Papapanagiotou <theofilos@gmail.com> * rename ecr registry directory for kserve Signed-off-by: Theofilos Papapanagiotou <theofilos@gmail.com> * merging kserve branch to master (kubeflow#35) * KServe Go module and API group change -------------------------------------- Updated go source files with new go module. Changed apigroup in yaml files. Made changes in Makefile to represent the repository and apigroup names. Verified `make test` is passing. Removed reference for old names. * Pointed correct repo reference for ko images (kubeflow#23) * Rename changes python - New (kubeflow#19) * python changes-initial-commit * updated api gen. Renamed package references in python. * Renamed python/kfserving to python/kserve * renamed python folder according to package kserve * Ran client-gen. Updated swagger.json * renamed constants from KFSERVING_ to KSERVE_. Yet to change namespace from kfserving-system to kserve and hence not changed in python side. * Updated kfserving reference to kserve Updated version for Python Package Index to 0.7.0rc0 Renamed KFServingSampleModel to KServeSampleModel * Python package and corresponding directory change has to be reflected in test and dockerfiles * updated a specific version in requirments to address issue with pip resolver. Added more specific versions in requirements,txt and updated paddleserver setup.py with kserve dependency. * Changed namespace from kfserving-system to kserve (kubeflow#31) * Update local dev scripts (kubeflow#33) * initial commit of dev-scripts update * Updated quick-install * Updated python Makefile * Generated install for new release * Updated generate-install script with new release * Reverting controller name in makefile. Will be updated in separate pr. * Github action changes (kubeflow#34) * Github action changes Migrated images publish to kserve repo. Added github workflows for the ones that was being pushed to gcr.io from release/triggers. * Removed batcher publisher since batcher.Dockerfile does not exist Updated tf2openapi.Dockerfile to correct directory and package. * Fixed issue with working directory of tf2openapi workflow * Removed the deptrecated logger docker * Excluded tests from linting. (kubeflow#36) They were pointing to older directory and package names and hence were not being excluded while linting. * Update license (kubeflow#20) * Fixed errors thrown by flake8 linter (kubeflow#37) * Kfserving kserve manifests changes and code reference changes (kubeflow#39) * kfserving -> kserve migartion initial commit * kfserving -> kserve config changes * Changed constants. Updated image reference from kserve to andyarok * Reverted kserve models web app image repo. * go changes for kfserving-kserve * Updated KFServing refernce in python code * Updated KFServing to KServe in python doc comments. Updated references of v1alpha2 from duplicate aliases to just use pkg name. Updated docs samples references with proper name. Made changes to use Constant for container name instead of literal "kserve-container". * Updated gitignore for generated files that came in after folder rename. Removed the generated files. Fixed lint warnings for go. * Removed travis yml file. * KServe doc update (kubeflow#38) * Update main and pythonserver readme * Update python docs * Fix role binding * Update kserve sdk doc * Fix e2e test script * Fix import * Kserve error fix (kubeflow#40) * Fixed linting errors in python * Fixed make test. Issue with storage initializer name/path. * Fixed failing python test. Updated ray version. Updated KFServingClient reference to KServeClient in kserve init. * Updated ray version from 1.4.0 to 1.5.0 * Fixed error with e2e due to kfserving reference in test overlay (kubeflow#42) Updated quick install and added 0.7.0-rc0 install manifests. Updated e2e namespace. * Controller repo update (kubeflow#45) Made changes in workflow to update image repo to kserve instead of kfserving * Controller repo update (kubeflow#45) Made changes in workflow to update image repo to kserve instead of kfserving * update pytorch image * Fix transformer dockerfile Co-authored-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> Co-authored-by: andyi2it <87992092+andyi2it@users.noreply.github.com> * Generate 0.7.0 release candidate and update roadmap/release process (kubeflow#46) * Fix apiVersion for examples and docs (kubeflow#55) Fix apiVersion for examples and docs change from serving.kubeflow.org to serving.kserve.io * add default deployment and fix startup without knative to master branch (kubeflow#57) * add default deployment and fix startup without knative * fix suite_test error * fix suit_test error * add constant vb for RawDeployment and Serverless * update predictor string RawDeployment to constants.RawDeployment * modify getDeploymentMode return type * Update kserve layer diagram (kubeflow#58) * update diag (kubeflow#59) * Remove kubeflow copyright (kubeflow#56) * Adding kfs_architect.drawio and changing kfs_architect.png's "KFserving" name to "KServe" (kubeflow#60) * Delete kfs_architect.png * Add files via upload Adding kfs_architect.drawio and changing kfs_architect.png's "KFserving" name to "KServe" * Delete kfs_architect.png * Delete kfs_architect.drawio * added Metric scrapping title * Delete kfs_architect.png * Delete kfs_architect.drawio * fixed typo * add Nick Hill as a reviewer (kubeflow#66) also removing rkelkar * Separate out web-app repo (kubeflow#68) * Sepate out web-app repo * Remove models web-app github action * Fix feast example Co-authored-by: Theofilos Papapanagiotou <theofilos@gmail.com> Co-authored-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> Co-authored-by: andyi2it <87992092+andyi2it@users.noreply.github.com> Co-authored-by: Chin Huang <chhuang@us.ibm.com> Co-authored-by: Qingshan Chen <38182824+Iamlovingit@users.noreply.github.com> Co-authored-by: Animesh Singh <singhan@us.ibm.com> Co-authored-by: js-ts <79689323+js-ts@users.noreply.github.com>

chore: Update third party license csv for viewercontroller

chore: Cherry-pick key commits for supporting v1 features in v2 API

Yasser Elsayed added 4 commits November 2, 2018 23:16

initial, moving paging into custom table, tests disabled

ec5a506

wip, rest of custom table occurrences, run list broken

bce08bb

fix run list and others

0d08bf6

tests

13dc178

yebrahim added the area/frontend label Nov 3, 2018

yebrahim requested a review from rileyjbauer November 3, 2018 06:20

k8s-ci-robot requested review from gaoning777 and jlewi November 3, 2018 06:20

k8s-ci-robot added the size/L label Nov 3, 2018

cleanup

1fbbd99

yebrahim force-pushed the yebrahim/paged-custom-table branch from d2ce2b5 to 1fbbd99 Compare November 3, 2018 07:19

rileyjbauer approved these changes Nov 3, 2018

View reviewed changes

pr comments

a53b313

yebrahim commented Nov 3, 2018

View reviewed changes

rileyjbauer reviewed Nov 3, 2018

View reviewed changes

k8s-ci-robot added the approved label Nov 4, 2018

k8s-ci-robot assigned rileyjbauer Nov 4, 2018

k8s-ci-robot added the lgtm label Nov 4, 2018

k8s-ci-robot merged commit e5cdc02 into master Nov 4, 2018

yebrahim deleted the yebrahim/paged-custom-table branch November 6, 2018 05:04

magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023

Fix TODOs and improve testing code (kubeflow#31)

f32cf05

HumairAK referenced this pull request in red-hat-data-services/data-science-pipelines Mar 11, 2024

Add reviewer from IBM China (#31)

dfaa3ef

HumairAK referenced this pull request in red-hat-data-services/data-science-pipelines Mar 11, 2024

Merge pull request #31 from gmfrasca/fix-backend-licenses

50b6dd5

chore: Update third party license csv for viewercontroller

HumairAK pushed a commit to HumairAK/data-science-pipelines that referenced this pull request May 10, 2024

Merge pull request kubeflow#31 from rimolive/rmartine

680e445

chore: Cherry-pick key commits for supporting v1 features in v2 API

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Push pagination control to CustomTable #31

Push pagination control to CustomTable #31

yebrahim commented Nov 3, 2018 •

edited by jlewi

Loading

rileyjbauer left a comment

yebrahim commented Nov 3, 2018

yebrahim left a comment

rileyjbauer left a comment

rileyjbauer commented Nov 4, 2018 •

edited

Loading

rileyjbauer commented Nov 4, 2018

k8s-ci-robot commented Nov 4, 2018

yebrahim commented Nov 4, 2018

k8s-ci-robot commented Nov 4, 2018

google-prow-robot commented Nov 4, 2018

rileyjbauer commented Nov 4, 2018

Push pagination control to CustomTable #31

Push pagination control to CustomTable #31

Conversation

yebrahim commented Nov 3, 2018 • edited by jlewi Loading

rileyjbauer left a comment

Choose a reason for hiding this comment

yebrahim commented Nov 3, 2018

yebrahim left a comment

Choose a reason for hiding this comment

rileyjbauer left a comment

Choose a reason for hiding this comment

rileyjbauer commented Nov 4, 2018 • edited Loading

rileyjbauer commented Nov 4, 2018

k8s-ci-robot commented Nov 4, 2018

yebrahim commented Nov 4, 2018

k8s-ci-robot commented Nov 4, 2018

google-prow-robot commented Nov 4, 2018

rileyjbauer commented Nov 4, 2018

yebrahim commented Nov 3, 2018 •

edited by jlewi

Loading

rileyjbauer commented Nov 4, 2018 •

edited

Loading