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

Adjust padding around icons in resources/ and dag view #1361

Merged
merged 1 commit into from
May 23, 2023
Merged

Conversation

kenxu95
Copy link
Contributor

@kenxu95 kenxu95 commented May 23, 2023

Describe your changes and why you are making these changes

I'm actually not sure if these are actually improvements anymore, but wanted to send it out just in case any of these appealed to you guys.

On the DAG view, for operators, I removed the padding so that there wasn't as large of a gap between the header icon the text.
On the resources/ page, I made everything justified with the left side since it looked a little funny, but I'm not sure it looks any better.

OLD:
image

NEW:
image


OLD:
image

NEW:

image

Related issue number (if any)

Loom demo (if any)

Checklist before requesting a review

  • I have created a descriptive PR title. The PR title should complete the sentence "This PR...".
  • I have performed a self-review of my code.
  • I have included a small demo of the changes. For the UI, this would be a screenshot or a Loom video.
  • If this is a new feature, I have added unit tests and integration tests.
  • I have run the integration tests locally and they are passing.
  • I have run the linter script locally (See python3 scripts/run_linters.py -h for usage).
  • All features on the UI continue to work correctly.
  • Added one of the following CI labels:
    • run_integration_test: Runs integration tests
    • skip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)

@agiron123
Copy link
Contributor

Nice work! These two are still a bit off center. The others look fine though.
image

Copy link
Contributor

@agiron123 agiron123 left a comment

Choose a reason for hiding this comment

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

Left a small comment about the file icons, but should be good to merge after that's resolved.

@kenxu95
Copy link
Contributor Author

kenxu95 commented May 23, 2023

@agiron123 yeah I think that's just cus that icon is cropped inconsistently from the others so it has a different width....

@kenxu95
Copy link
Contributor Author

kenxu95 commented May 23, 2023

Going to merge this as-in since it's a strict improvement for now.

@kenxu95 kenxu95 added the skip_integration_test Skips required integration test (only documentation/UI changes) label May 23, 2023
@kenxu95 kenxu95 merged commit 2cdd1bc into main May 23, 2023
@kenxu95 kenxu95 deleted the padding_fix branch May 23, 2023 23:24
@kenxu95 kenxu95 restored the padding_fix branch May 23, 2023 23:25
@kenxu95 kenxu95 mentioned this pull request May 23, 2023
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip_integration_test Skips required integration test (only documentation/UI changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants