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

Make grid view widths adjustable #27273

Merged
merged 3 commits into from
Oct 26, 2022
Merged

Conversation

bbovenzi
Copy link
Contributor

Make the border between the grid and details panel draggable so users can adjust the width as they want.

Oct-25-2022 12-42-14
Oct-25-2022 08-24-36


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Oct 25, 2022
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Very cool feature !

I'll do a more in depth review tomorrow. (can't test the code right now)

Overall looking good.

I feel like we have quite a few 'magic numbers' that are hard to reason about. Most of them were not introduced in this PR, but maybe extracting them into shared variables with explicit names could help in maintaining them later ?

useEffect(() => {
if (contentRef.current) {
const topOffset = contentRef.current.offsetTop;
const footerHeight = parseInt(getComputedStyle(document.getElementsByTagName('body')[0]).paddingBottom.replace('px', '').replace('em', ''), 10) || 0;
Copy link
Member

Choose a reason for hiding this comment

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

This feels weird to replace indifferently px and em. I think footerHeight won't have the correct value if it is specified in 'em'. (We use it as a pixel value later in the code).

airflow/www/static/js/dag/Main.tsx Outdated Show resolved Hide resolved
airflow/www/static/js/dag/Main.tsx Outdated Show resolved Hide resolved
airflow/www/static/js/dag/grid/index.tsx Outdated Show resolved Hide resolved
@potiuk
Copy link
Member

potiuk commented Oct 26, 2022

❤️

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Oct 26, 2022

Tested locally and working great.

I just have one issue when displaying details for a task group with long 'tooltip'. There is a weird 'jump' when trying to resize at first. (a gif would have been better, sorry :p)

Starting from this:
image

And then when clicking on the separator for a rezise, it flickers, and the separator jumps to the left right away:
image

As if the tooltip was taking all the space and pushing as much as possible the separator to the left.

This could be just on my side and cause by my local setup:

  • Chrome Version 103.0.5060.134 (Official Build) (64-bit)
  • Ubuntu 20.04.4 LTS

@bbovenzi
Copy link
Contributor Author

bbovenzi commented Oct 26, 2022

I cleaned up the magic numbers.
I changed how the flex works to not have the issue with the group tooltip description.

@potiuk
Copy link
Member

potiuk commented Oct 26, 2022

Making Airflow UI better one piece at-a-time :)

@bbovenzi bbovenzi merged commit 49e6180 into apache:main Oct 26, 2022
@bbovenzi bbovenzi deleted the adjutable-grid-width branch October 26, 2022 21:13
@bbovenzi bbovenzi added this to the Airflow 2.5.0 milestone Oct 26, 2022
@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants