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.

Sign up for GitHub

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

Jump to bottom

feat(ui): pipeline UX improvements #2904

Merged
merged 10 commits into from
Nov 8, 2024
Merged

feat(ui): pipeline UX improvements #2904

merged 10 commits into from
Nov 8, 2024

Conversation

Marvin9
Copy link
Contributor

@Marvin9 Marvin9 commented Nov 6, 2024 •
edited
Loading

Screen.Recording.2024-11-07.at.4.02.06.PM.mov

@Marvin9
feat(ui): pipeline drag ux
89f1aa9 
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
@Marvin9
feat(ui): use transform for performance and fix overlap scroll bug
58da1fc 
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
@Marvin9
feat(ui): use scale to zoom for perf
bd85c05 
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
@Marvin9
feat(ui): fit to view and canvas zoom in/out
56b2b17 
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
@netlify Netlify
Copy link

netlify bot commented Nov 6, 2024 •
edited
Loading

✅ Deploy Preview for docs-kargo-io canceled.

Name Link
🔨 Latest commit f6c75e8
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-io/deploys/672e4dfb9ca60e00088f9501

@Marvin9 Marvin9 self-assigned this Nov 6, 2024
@codecov Codecov
Copy link

codecov bot commented Nov 6, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.92%. Comparing base (7813eff) to head (42f78a1).
Report is 3 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2904      +/-   ##
==========================================
- Coverage   50.02%   49.92%   -0.10%     
==========================================
  Files         275      275              
  Lines       24559    24668     +109     
==========================================
+ Hits        12285    12315      +30     
- Misses      11603    11676      +73     
- Partials      671      677       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Marvin9
chore(ui): improve animation
fa01aac 
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
@Marvin9
feat(ui): zoom preference
20ef3f0 
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
@Marvin9 Marvin9 marked this pull request as ready for review November 7, 2024 19:16
@Marvin9 Marvin9 requested a review from a team as a code owner November 7, 2024 19:16
@Marvin9 Marvin9 requested review from krancour and rpelczar November 7, 2024 19:22
@Marvin9
chore(ui): add toolbar for hint
d458cdd 
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
jessesuen
jessesuen approved these changes Nov 7, 2024
View reviewed changes
Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

Not a code review, but approval based on behavior demonstrated in video

krancour
krancour approved these changes Nov 7, 2024
View reviewed changes
Copy link
Member

@krancour krancour left a comment

Choose a reason for hiding this comment

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

🔥 This is amazing and I also approve based on a review of the functionality.

idk if @rpelczar may wish to review implementation details as well since this appears non-trivial.

rpelczar
rpelczar approved these changes Nov 7, 2024
View reviewed changes
Copy link
Contributor

@rpelczar rpelczar left a comment

Choose a reason for hiding this comment

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

Looks amazing! I left one comment with one small thing to improve.

ui/src/utils/use-temporary.ts Outdated Show resolved Hide resolved
@rpelczar
Copy link
Contributor

rpelczar commented Nov 8, 2024

I also think the save button is overkill. I like it, but not sure if it is as useful as the reset button.
This is my personal opinion, so you don't have to do anything with that.

@Marvin9
feat(ui): autosave zoom pref
f99a565 
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
@Marvin9
Copy link
Contributor Author

Marvin9 commented Nov 8, 2024

I think that makes sense @rpelczar, we have reset view anyways so I have removed explicit save view preference and it autosaves now. Let me know if you think otherwise @jessesuen @krancour

@Marvin9
chore(ui): improve layout and perf
42f78a1 
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
@Marvin9
fix(ui): perf updates, block child event propagation that cause re-re…
f6c75e8 
…nder whole pipeline

Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
@Marvin9 Marvin9 added this pull request to the merge queue Nov 8, 2024
Merged via the queue into main with commit d50c5bc Nov 8, 2024
15 checks passed
@Marvin9 Marvin9 deleted the Marvin9/feat-pipeline-drag-like-maps branch November 8, 2024 18:25
@02strich
Copy link

Btw, this breaks kargo on firefox due to https://developer.mozilla.org/en-US/docs/Web/API/Element/computedStyleMap not being supported there today

@hiddeco
Copy link
Contributor

hiddeco commented Nov 11, 2024

@02strich, thank you for bringing this to our attention. We've noticed this issue internally as well, and will work on a solution. Appreciate the report! 🙇

hiddeco added a commit to hiddeco/kargo that referenced this pull request Nov 11, 2024
@hiddeco
Revert "feat(ui): pipeline UX improvements (akuity#2904)"
8cb9412 
This reverts commit d50c5bc to make
the UI usable again for testing until the issues in `main` have been
resolved.
@rpelczar rpelczar mentioned this pull request Nov 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jessesuen jessesuen jessesuen approved these changes

@rpelczar rpelczar rpelczar approved these changes

@krancour krancour krancour approved these changes

Assignees

@Marvin9 Marvin9

Labels
area/ui kind/enhancement priority/normal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Marvin9 @rpelczar @02strich @hiddeco @krancour @jessesuen