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

Moved conditional rendering for breadcrumbs #3342

Conversation

FrancescoBrunoDev
Copy link
Contributor

@FrancescoBrunoDev FrancescoBrunoDev commented Jan 25, 2024

Check out #3328 for more information.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @FrancescoBrunoDev -- one question/suggestion. I don't have a strong feeling about which approach is better, but I think either way, a minor adjustment is needed.

themes/bootstrap3/templates/layout/layout.phtml Outdated Show resolved Hide resolved
@FrancescoBrunoDev
Copy link
Contributor Author

Great! So now it should be correct. I'm just not sure about the indentation because you mentioned it needed to be de-indented by two spaces, but perhaps you meant just one?

@demiankatz
Copy link
Member

Thanks, @FrancescoBrunoDev. The indentation looks good to me. I'll test this as soon as I can and hopefully get it merged in the near future.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

This looks good to me. I made a small adjustment -- parentheses needed to be added around the showBreadcrumbs check because of the operator precedence of ?? and &&. Eventually it may make sense to eliminate showBreadcrumbs entirely, because as far as I can tell, it is never used... but at least things are generally simplified here!

Thanks again for making this improvement.

@demiankatz demiankatz merged commit 8e33e25 into vufind-org:dev Jan 31, 2024
7 checks passed
@demiankatz demiankatz added this to the 10.0 milestone Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants