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

Improvement: Add the option to suppress the footer button, solves #444. #464

Merged
merged 2 commits into from
Dec 6, 2023

Conversation

lucaboesch
Copy link
Collaborator

@lucaboesch lucaboesch commented Nov 8, 2023

I've been given the code mentioned here #6 (comment) thanks to @goggo24 and @moniNaydenov and I have built upon that one.
Thanks heaps!

@lucaboesch lucaboesch requested a review from abias November 8, 2023 10:38
@lucaboesch lucaboesch force-pushed the suppressfooter branch 7 times, most recently from 2afb4ba to 515fdff Compare November 12, 2023 20:07
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 @lucaboesch ,

many thanks for working on this contribution!
And, of course, many thanks to @goggo24 and @moniNaydenov for doing the initial implementation!

I just had a look at the patch and did some polishing and alignment in a review commit (you will see what I did if you look at the diff), but, regarding the plain functionality, I am afraid that we are not quite there yet.

The new setting differentiates between Desktop+Mobile, Mobile only, Desktop only and Completely hidden. This is great.
Hiding the button completely is done is done in the mustache template, the hiding on the particular device sizes are done with CSS. This perfectly fine as well.

But here are my questions / concerns:

  • Maybe I did something wrong, but in my manual tests only the setting to completely hide the footer button (which is realized in mustache) worked. The other settings did not have any real effect for me on different screen sizes. Regardless if I used your original code or my polished code. Could you please double check these options?
  • I am wondering why the CSS code to show / hide the footer button targets two different selectors #page-footer [data-region="footer-container-popover"] .btn-footer-popover and #page-footer .footer-content-popover.container differently. As I understand it, both DOM elements should be handled / shown / hidden in the same way. Could you please clarify that?
  • I would like to add a (unchanged as presented by Moodle core) suffix to one of the setting options, similar to the "theme_boost_union | navbarcolor" setting. This way, admins should be able to identify quickly which option to choose if they do not want to change Boost core at all. The natural choice for me would be "Desktop + Mobile". But are you aware that Boost core does not show the footer button on really small devices at all? Thus, the default should be "Desktop only", shouldn't it?
  • Against the same background: As Boost core does not show the question mark icon on really small devices, should we re-show it in Boost Union if the admin chooses "Desktop + Mobile" or "Mobile only"? I understand the code that this is intended (and just not working for me), but it might be worthwhile then to add this fact to the setting description.
  • Against the same background: I think the default in the switch within the theme_boost_union_get_scss_to_hide_footer_button() function should return the CSS code for the default setting, not for the "None" setting as it does now.

Looking forward to your feedback!

PS: I did not touch and check the Behat tests up to now as I wanted to discuss these items first.

Thanks,
Alex

@lucaboesch
Copy link
Collaborator Author

Some /* stylelint-disable-line declaration-no-important */ to add but then it is good.

@abias
Copy link
Member

abias commented Dec 6, 2023

After chatting to Luca 1:1 several days ago, I went ahead and tried to solve the remaining issues myself.
The cause for the issue that I did not see any effect of the settings initially was in my development environment, this is solved now.

Beyond this initial issue for my review, I have handled all other review items myself. The main challenge was to cover all possible permutations of resulting positions for the footer button, communications button and back to top button together with the sticky footer and the smart menu bottom bar. I hope that I covered everything correctly even though the CSS code has become quite lenghty now.

@abias
Copy link
Member

abias commented Dec 6, 2023

I will merge this now, but I will not squash the commits (as I normally do) as the review changes have become quite heavy and I think it would be better to distinguish my work from @@moniNaydenov's work in the commit history.

@abias abias merged commit 7bbdf1a into moodle-an-hochschulen:master Dec 6, 2023
6 checks passed
@lucaboesch lucaboesch deleted the suppressfooter branch December 19, 2023 07:33
detomon pushed a commit to detomon/moodle-theme_boost_union that referenced this pull request Aug 26, 2024
…footer

Improvement: Add the option to suppress the footer button, solves moodle-an-hochschulen#444.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: CLOSED
Development

Successfully merging this pull request may close these issues.

Improvement: Allow for complete removal of the "footer" (question mark in circle) button
2 participants