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

Bugfix: Correct order for in-course breadcrumb when sections exist in it (First categories then sections) #483

Merged

Conversation

danowar2k
Copy link
Collaborator

Solves #317

Copy link
Member

@abias abias left a comment

Choose a reason for hiding this comment

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

Hi @danowar2k , many thanks for working on this bug!

The fact that the order is wrong in the breadcrumb is clearly a bug and your patch solves this bug.

I have just rebased your branch onto latest master, have generalized the Behat steps which have been introduced by you a little bit and have pushed it to your branch again.

However, I have one questions regarding your Behat scenario which I would like to ask you to answer before we merge this.

In addition to that, I have raised #593 and #594 as follow-up issues. I would be grateful if you could have a look at these as well.

@abias
Copy link
Member

abias commented Apr 1, 2024

@danowar2k - I do not want to bug you, but I would appreciate your answer to the question above. As soon as I have understood your intention, I will be able to finish the review and merge the PR.

@danowar2k
Copy link
Collaborator Author

Whoops, completely overlooked this among the many other mails I'm getting from Boost Union on Github. I should probably change my notification settings... Sorry, will look into it

@abias abias force-pushed the #317-section-breadcrumb-order branch from 1be27b9 to 997e019 Compare April 20, 2024 13:58
@abias
Copy link
Member

abias commented Apr 20, 2024

Thanks, @danowar2k , I think I got your point now.
I have amended the Behat scenario to test the placement of the "Enrolment options" string for non-logged-in users as well.
This is fine now from my point of view, I will merge the PR as soon as the tests are green.

Regarding your comment about the fact that "Enrolment options" string could be a link: I think this would be solved as soon as #594 would be solved. In this case, the course main page would be shown between the category name and the "Enrolment options" strings as an additional breadcrumb item. If you have some spare time, I would be grateful if you could look into that issue.

Cheers,
Alex

@abias abias merged commit c1d1a57 into moodle-an-hochschulen:master Apr 21, 2024
6 checks passed
abias pushed a commit that referenced this pull request Apr 21, 2024
abias pushed a commit that referenced this pull request Apr 21, 2024
abias pushed a commit that referenced this pull request Apr 21, 2024
detomon pushed a commit to detomon/moodle-theme_boost_union that referenced this pull request Aug 26, 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.

Bug: Breadcrumb navigation is partial in wrong order
2 participants