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

Dendrogram pickup - code review 2 #282

Open
wants to merge 9 commits into
base: dendrogram-pickup
Choose a base branch
from

Conversation

JamesxX
Copy link
Contributor

@JamesxX JamesxX commented Oct 24, 2023

Made most of the changes requested in the code review. There's still some more work to be done most likely, but it got a bit tricky seeing what each message was referring to given the amount of changes made

@JamesxX JamesxX mentioned this pull request Oct 24, 2023
Copy link
Member

@fenjalien fenjalien left a comment

Choose a reason for hiding this comment

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

More comments sorry 🙃 nice work though!

Could you tidy up the formatting of the if statements please? The conditional doesn't need brackets and always put the result on a new line (unless its an inline statement).

if conditional {
  result
}

Also I would prefer a single assignment for conditonal assignments. So instead of

let a = 0
if b {
  a = 1
}

do

let a = if b { 1 } else { 0 }
// or
let a = if b { 
  1
} else {
  0
}

src/lib/chart/dendrogram.typ Outdated Show resolved Hide resolved
src/lib/chart/dendrogram.typ Show resolved Hide resolved
src/lib/chart/dendrogram.typ Outdated Show resolved Hide resolved
@JamesxX
Copy link
Contributor Author

JamesxX commented Oct 24, 2023

More comments sorry 🙃 nice work though!

I appreciate the candid and constructive feedback. I'm still new to Typst, and haven't written a single line of rust, so its useful to learn appropriate styles and code-smells to avoid

@JamesxX
Copy link
Contributor Author

JamesxX commented Oct 24, 2023

Getting there!
image

@johannes-wolf
Copy link
Member

I'd like to implement and use polar axes for this. Maybe I'll find some time during the weekend, as I am not really happy about how the "radial" mode looks with the Cartesian axes right now.

@JamesxX
Copy link
Contributor Author

JamesxX commented Oct 26, 2023

I'd like to implement and use polar axes for this. Maybe I'll find some time during the weekend, as I am not really happy about how the "radial" mode looks with the Cartesian axes right now.

Definitely agreed. Cartesian axes are only there because of a lack of alternative at the moment. Polar axes would be great though!

@johannes-wolf johannes-wolf added this to the 0.3.0 milestone Jan 17, 2024
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.

None yet

3 participants