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

Update nav.html #3320

Closed
wants to merge 1 commit into from
Closed

Update nav.html #3320

wants to merge 1 commit into from

Conversation

Ayush277
Copy link

Using unless makes the condition slightly more readable by directly expressing the condition for the exception. In this case, we want to render the breadcrumb unless the layout is 'index', which reads more naturally and reduces cognitive load compared to checking if the layout is not 'index'. This refactor does not alter the behavior of the code but enhances its clarity and maintainability.

@Ayush277
Update nav.html
11ed158 
Using unless makes the condition slightly more readable by directly expressing the condition for the exception
@Ayush277 Ayush277 requested a review from a team as a code owner October 20, 2024 11:23
@jmeridth
Copy link
Member

@Ayush277 thank you for the PR. Looks like we need to switch from endif to endunless to pass CI.

In my personal opinion, I do not think unless is more readable. I think it causes more confusion because we have to negate in our heads instead of explicit !=. But that is a religious argument between rubyists and non-rubyists. 😉 Since we're using liquid for the templates, I get it.

I will ping the other maintainers and get some reviews on this. Again, thank you for the contribution.

@Satishmirjan
Copy link

assign me this , i already solved similar problems.

@jmeridth
Copy link
Member

assign me this , i already solved similar problems.

This is a pull request, already assigned/owned. Thank you though.

@Evilwa1

This comment has been minimized.

Sign in to view
@kenyonj
Copy link
Contributor

kenyonj commented Oct 21, 2024 •
edited
Loading

@Ayush277 thank you for the PR. Looks like we need to switch from endif to endunless to pass CI.

In my personal opinion, I do not think unless is more readable. I think it causes more confusion because we have to negate in our heads instead of explicit !=. But that is a religious argument between rubyists and non-rubyists. 😉 Since we're using liquid for the templates, I get it.

I will ping the other maintainers and get some reviews on this. Again, thank you for the contribution.

I agree.

For me, this is more readable:
If the condition is not true, then do your thing
than this:
Unless the condition is true, then do your thing

It always takes me a 🤌 little bit more to figure out if I fall into the condition funnel with the unless

@jmeridth
Copy link
Member

Thank you for consensus @kenyonj @tomthorogood. Please continue to contribute @Ayush277 🙇

@jmeridth jmeridth closed this Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Ayush277 @jmeridth @Satishmirjan @Evilwa1 @kenyonj