Skip to content

Conversation

@ferenc-csaky
Copy link
Contributor

@ferenc-csaky ferenc-csaky commented Apr 28, 2022

What is the purpose of the change

Fixes hidden scrollbar for some UI elements, where CSS calculate was based on a percentage. The current Angular version has a bug, so in those cases the scrollbar is hidden and scrolling is not possible.

Relevant Angular issue: NG-ZORRO/ng-zorro-antd#3090

Brief change log

Changed calc ( X% ...) to calc( Xvh ...), which solves this problem.

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

How to test

Prepare a Flink job with a number of Accumulators that cannot fit to the tab, so scrolling is required. After running the job, the "Accumulators" tab has no scrollbar. The CSS can be changed in the browser, so it can be modified on the fly to try out the fix.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changed class annotated with @Public(Evolving): no
  • The serializers: no
  • The runtime per-record code paths (performance sensitive): no
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? no

@flinkbot
Copy link
Collaborator

flinkbot commented Apr 28, 2022

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@ferenc-csaky
Copy link
Contributor Author

@flinkbot run azure

@gaborgsomogyi
Copy link
Contributor

gaborgsomogyi commented Apr 29, 2022

The change looks good basically but I've some minor comments.

The current Angular version has a bug

Do we have a link for the bug? Please add it to the PR.

I would mention manual testing steps in the "Verifying this change" area. Namely on which page which UI element needs to be checked (I know it but it would be good to let others know).

Could you attach a before and after picture just to double check that we see the same thing?

I've started the manual testing...

@gaborgsomogyi
Copy link
Contributor

Oh and almost forgot, please add component to the title of the PR like [FLINK-27441][webfrontend]...

@ferenc-csaky
Copy link
Contributor Author

ferenc-csaky commented Apr 29, 2022

Thanks for the comments.

  1. Yes, I linked it to the corresponding Jira, but here it is: Percentage or css:'calc()' should be added in 'nzScroll' NG-ZORRO/ng-zorro-antd#3090
  2. I tested this with Accumulators. I prepared a dummy Flink job and I added 50 dummy IntCounter accumulators to it, so it would not fit to the screen. After I started the job, the "Accumulators" tab has no scrollbar. The CSS can be changed in the browser, so it is easy to test this on the fly.

Original state:
Screenshot 2022-04-29 at 10 55 55

Fixed state:
Screenshot 2022-04-29 at 11 04 14

@ferenc-csaky ferenc-csaky changed the title [FLINK-27441] fix "nzScroll" calculations [FLINK-27441][webfrontend] fix "nzScroll" calculations Apr 29, 2022
Copy link
Contributor

@gaborgsomogyi gaborgsomogyi left a comment

Choose a reason for hiding this comment

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

LGTM. Tested manually and works as expected.

@gaborgsomogyi
Copy link
Contributor

@mbalassi @gyfora could you have a look plz?

@mbalassi
Copy link
Contributor

mbalassi commented Apr 29, 2022

Thanks @ferenc-csaky and @gaborgsomogyi. Will merge on Monday after a quick test unless anyone disagrees.

@mbalassi mbalassi self-requested a review April 29, 2022 17:09
Copy link
Contributor

@mbalassi mbalassi left a comment

Choose a reason for hiding this comment

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

LGTM

@mbalassi mbalassi merged commit f3d4ceb into apache:master May 2, 2022
@ferenc-csaky ferenc-csaky deleted the FLINK-27441 branch May 29, 2025 15:25
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.

4 participants