Remove automatically scrolling into view functionality #1646
Conversation
|
@vconzola - Can you take a look at this? I think you were the one that worked on the design originally. |
|
@DaoDaoNoCode I was the one who requested the auto-scroll. The reason it was added is that when a user deploys a model the only feedback they get today is that the number of deployed models in the model server row increments by one. (I thought that the row was supposed to auto-expand to show the deployed models in the row expansion, but it doesn't appear this was implemented, or it was implemented and later removed. I swear it used to do that, but doesn't anymore.) So we need a way to make it more obvious that a model was deployed. My thought was that if we auto-scrolled to the row expansion for deployed models the newly deployed model would be easily visible. If this is a problem from a code perspective, we need to figure out another solution for making newly deployed models visible. |
|
@vconzola I can make the auto expand happen when you newly deploy a model. |
|
@DaoDaoNoCode The problem I was seeing before is that even with auto-expand when a new model was deployed the expansion area was displayed "below the fold" so the user didn't see it without scrolling and therefore didn't know the model was deployed. Can we do any sort of auto-scrolling to ensure the expansion is visible without the user needing to manually scroll? Or is auto-scrolling in general not an option? |
|
@vconzola Let me give it a try. |
DaoDaoNoCode
force-pushed
the
upstream-issue-1545
branch
from
c01ae95
to
2c7a13e
Compare
DaoDaoNoCode
force-pushed
the
upstream-issue-1545
branch
from
2c7a13e
to
84c167e
Compare
|
@lucferbux Haha I guess it was me who wrote this erroneous functionality... But I cannot recall what I was thinking at that time 🤷♂️ |
|
/lgtm Working on my end and code looks good |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: Gkrumbach07, lucferbux 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:
Approvers can indicate their approval by writing |
| <ScrollViewOnMount | ||
| shouldScroll={activeColumn === ServingRuntimeTableTabs.DEPLOYED_MODELS} | ||
| /> |
There was a problem hiding this comment.
Curious why you didn't just unmount it...
There was a problem hiding this comment.
Hmm, I totally forgot I could unmount it...
Closes #1545
Description
The div will automatically scroll into view because we added that feature somehow. But I think it's not needed because users can always scroll the screen by themselves, auto-scroll will only destroy some of the layouts and behaviors and cause unexpected results.
@kywalker-rh Could you confirm this?
The context: When you click on
Deployed modelson project details page, it will expand the models table, and the page will scroll to show the whole table. However, this auto scroll could cause problems. So I removed it, no when clicking onDeployed models, the page will not auto scroll anymore. Is that acceptable?How Has This Been Tested?
Follow the same instructions in the issues, you will find the issue disappeared.
Test Impact
N/A, layout issue.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main