Skip to content

Comments

Update onboarding bracket to be robust#6099

Merged
jessfraz merged 10 commits intomainfrom
josh/update-onboarding-bracket
Apr 11, 2025
Merged

Update onboarding bracket to be robust#6099
jessfraz merged 10 commits intomainfrom
josh/update-onboarding-bracket

Conversation

@jgomez720
Copy link
Contributor

@jgomez720 jgomez720 commented Apr 1, 2025

Updated the onboarding bracket to be more robust. Now it:

  • has better defined parameters so when users change values that bracket doesn't fail or look unrealistic
  • has asserts to check lengths/widths to make sure output is possible
  • Updated sketch/extrude vars to be more descriptive for TTC
  • has @settings for units

Just want some ME feedback if this would be understood to others. assert is going to confuse designers, but this is a lesson I'd rather them learn

@qa-wolf
Copy link

qa-wolf bot commented Apr 1, 2025

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

@vercel
Copy link

vercel bot commented Apr 1, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview Apr 11, 2025 6:43pm

@github-actions
Copy link
Contributor

github-actions bot commented Apr 1, 2025

src/lib/exampleKcl.ts has been updated in this PR, please review and update the src/routes/onboarding, if needed.

Copy link
Contributor

@nicboone8 nicboone8 left a comment

Choose a reason for hiding this comment

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

Is using the bend radius as the fillet radius confusing, either to a user or to T2C?

We'd talked about breaking the onboarding up into smaller tutorials, each focused on a different sample. That would give us some room to start users off with something easy before throwing a ton of new concepts at them. I think using assert is totally fine if it ends up in tutorial 3 or 4-ish

@jgomez720
Copy link
Contributor Author

Yeah that could be confusing. I'll make it more clear.

For the onboarding, I agree with you in giving users information piece by piece.
cc @franknoirot

@franknoirot
Copy link
Contributor

This closes #4827

@franknoirot
Copy link
Contributor

Yup @nicboone8 @jgomez720 I have #5986 on my task list to support multiple onboarding flows. I think the code itself could include assert but save explaining it for a later more advanced tutorial

Copy link
Contributor

@nickmccleery nickmccleery 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 dope, I didn't even know we had assert statements in KCL. That's a really nice feature.

@pierremtb
Copy link
Contributor

Nice! Will have to make a follow-up PR to #5946 to ensure we can edit with the radius of non-pipe fillets like we have here with the right-click > Edit flow. Just created the issue at #6119

@jessfraz
Copy link
Contributor

jessfraz commented Apr 3, 2025

I think this broke a test, also maybe one of the front end engineers could help you , we should load this from the bracket/main.kcl versus having two files, thats annoying cc @franknoirot @pierremtb who might be able to help

@jessfraz
Copy link
Contributor

jessfraz commented Apr 3, 2025

the export test failure seems real so while someone is fixing that we might as well fix how bracket is loaded

@jessfraz
Copy link
Contributor

jessfraz commented Apr 4, 2025

I can help you fix this tomorrow josh

@pierremtb
Copy link
Contributor

Hit the update button cause the export flakiness was improved with 45e5b25 last night

@jessfraz
Copy link
Contributor

jessfraz commented Apr 4, 2025

ah okay i still wanna fix the bracket being two places but we can do out of band

@jessfraz
Copy link
Contributor

jessfraz commented Apr 4, 2025

my ocd ccant handle it

@pierremtb
Copy link
Contributor

Updating as #6164 went in and should fix some if not all issues here

@pierremtb
Copy link
Contributor

Crap I have a feeling e2e/playwright/fixtures/bracket.ts fails on windows and that leads to those 3 real failures

@pierremtb
Copy link
Contributor

I'm on windows now so I can help debug this

@jessfraz
Copy link
Contributor

jessfraz commented Apr 7, 2025

@pierremtb yeah it looked like it was reading the file but something about the noNewLinesBracket is different

@pierremtb
Copy link
Contributor

pierremtb commented Apr 7, 2025

I think I was wrong, the tests that were failing at https://github.com/KittyCAD/modeling-app/actions/runs/14281282402/job/40032997357 are all passing locally on my windows laptop 🤔

@jessfraz
Copy link
Contributor

jessfraz commented Apr 7, 2025

oh thats good maybe ci is just being a butt

@jgomez720
Copy link
Contributor Author

dear god what happened to this

@pierremtb
Copy link
Contributor

Yeah those tests are busted in CI, will have another look tomorrow

@jessfraz
Copy link
Contributor

ill rebase and fix this pr tomorrow

@pierremtb
Copy link
Contributor

@jessfraz Tried to give you a headstart, did git merge main -X theirs and just redo-sim-tests. Feel free to revert if I broke something

jgomez720 and others added 9 commits April 11, 2025 09:19
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Signed-off-by: Jess Frazelle <github@jessfraz.com>
@jessfraz jessfraz merged commit 74c07fc into main Apr 11, 2025
42 checks passed
@jessfraz jessfraz deleted the josh/update-onboarding-bracket branch April 11, 2025 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants