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: Bulk actions widget overlaid course header image, solves #469. #471

Merged
merged 1 commit into from
Nov 12, 2023

Conversation

abias
Copy link
Member

@abias abias commented Nov 11, 2023

Before:
grafik

After:
grafik

This was a quick fix in CSS, but it should be sufficient from my point of view.

While the "bulk actions" widget on Moodle 4.3 is the first widget where I realized this header actions area myself, the header actions area has been there before on Moodle 4 already and might be filled by course formats or plugins. Thus, it should be safe to backport this patch to pre-4.3 versions of Boost Union as well.

@abias abias requested a review from lucaboesch November 11, 2023 07:41
lucaboesch
lucaboesch previously approved these changes Nov 11, 2023
Copy link
Collaborator

@lucaboesch lucaboesch left a comment

Choose a reason for hiding this comment

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

I see what you are doing there, and I am willing to approve this (in fact I do) but the more proper way would be to add dynamically add a .button-secondary class to it or load button-secondary SCSS mixins, I think.

@abias
Copy link
Member Author

abias commented Nov 12, 2023

Thank you for your approval, Luca.
I understand your concerns. However, I think that we cannot rely on the fact that a button is displayed in this header actions area. I looked a little bit closer and saw that it just needs a all to $PAGE->add_header_action() to show content in this area.

That's why I just made sure that the content, whateever it is, is displayed on a white background just as Boost Core does it.

I will now rebase the branch and merge it as you have approved it.

Cheers

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.

2 participants