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

Dynamically adjustment for footer height #34391

Closed

Conversation

RyanAveritt
Copy link

@RyanAveritt RyanAveritt commented Oct 3, 2022

Footer height is dynamically adjusted to 95px if one of the legal links is set; originally 65px. (first-time contributor)
Fix #34305

@solracsf solracsf mentioned this pull request Oct 3, 2022
@solracsf solracsf requested a review from skjnldsv October 3, 2022 08:35
@solracsf solracsf changed the title Issue#34305: Added dynamically adjustment for footer height Dynamically adjustment for footer height Oct 3, 2022
@solracsf solracsf added this to the Nextcloud 26 milestone Oct 3, 2022
@solracsf solracsf added the 3. to review Waiting for reviews label Oct 3, 2022
@RyanAveritt
Copy link
Author

Checking in; is there anything else I need to do? Also, any feedback and/or advice would be highly welcomed.

@@ -21,6 +21,9 @@
*/

window.addEventListener('DOMContentLoaded', function () {

//dynamically changes footer size depending on presence of links
if(document.querySelectorAll("a.legal").length > 0) document.getElementById("body-public").querySelector('footer').style = 'height: 95px'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(document.querySelectorAll("a.legal").length > 0) document.getElementById("body-public").querySelector('footer').style = 'height: 95px'
if (document.querySelectorAll('a.legal').length > 0) {
document.getElementById('body-public').querySelector('footer').style = 'height: 95px'
}

Copy link
Member

Choose a reason for hiding this comment

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

But I think @szaimen have another fix somewhere?
I'm not super sure this is the way to go 🤔

@jotoeri
Copy link
Member

jotoeri commented Oct 16, 2022

Wouldnt it be better to have a class footer--large or so in public.css and insert the class (dynamically) on the template? That would insert it from ground up instead of altering the height afterwards. 🤔

/* public footer */

@szaimen
Copy link
Contributor

szaimen commented Oct 16, 2022

+1 for the global css variable approach

@skjnldsv
Copy link
Member

+1 for the global css variable approach

Careful about too much variables, we're defeating the initial goal of them

@jotoeri
Copy link
Member

jotoeri commented Oct 16, 2022

+1 for the global css variable approach

Careful about too much variables, we're defeating the initial goal of them

Nah, was not thought as global variable (from my side), but just a class overriding the height:

footer {
    height: 65px;

    .footer--large {
        height: 80px;
    }
}

@szaimen
Copy link
Contributor

szaimen commented Oct 16, 2022

+1 for the global css variable approach

Careful about too much variables, we're defeating the initial goal of them

Right. However since we have mutliple places where we need to reuse that one, I guess adding one for that is probably the best idea...

@jotoeri
Copy link
Member

jotoeri commented Oct 16, 2022

However since we have mutliple places where we need to reuse that one, I guess adding one for that is probably the best idea...

Could you point to where they are calculated and set?

@szaimen
Copy link
Contributor

szaimen commented Oct 16, 2022

@jotoeri
Copy link
Member

jotoeri commented Oct 16, 2022

See https://github.com/nextcloud/server/search?q=%24footer-height

Uhm, maybe i understood wrong, but aren't these just variables within the scss sheet, but will be compiled to fixed numbers within the finally loaded css? Which is sth. different than global css variables var(--footer-height), that could be used within apps then?

@szaimen
Copy link
Contributor

szaimen commented Oct 16, 2022

Uhm, maybe i understood wrong, but aren't these just variables within the scss sheet, but will be compiled to fixed numbers within the finally loaded css? Which is sth. different than global css variables var(--footer-height), that could be used within apps then?

indeed. That is why I would push towards a global css variable that could get reused or changed

@jotoeri
Copy link
Member

jotoeri commented Oct 16, 2022

That is why I would push towards a global css variable that could get reused or changed

Ok 👍 . But then my question was related to give a hint to RyanAveritt on where the variable var(--footer-height) can be calculated (depending on the footer-content) and inserted into the loaded variables. This i don't know currently by my own, so i can't tell anything on that. Probably somewhere in the theming app, which also produces the footer-content?

@szaimen
Copy link
Contributor

szaimen commented Oct 16, 2022

Ok 👍 . But then my question was related to give a hint to RyanAveritt on where the variable var(--footer-height) can be calculated (depending on the footer-content) and inserted into the loaded variables. This i don't know currently by my own, so i can't tell anything on that. Probably somewhere in the theming app, which also produces the footer-content?

I don't know if we can make a logic from that. Rather changing the varialbe to e.g. 95px if impressum gets activated?

@jotoeri
Copy link
Member

jotoeri commented Oct 16, 2022

Rather changing the varialbe to e.g. 95px if impressum gets activated?

Yeah, thats what i meant. 😄 Just probably a bit bad formulated. Having a variable, that is either 65px or 95px. (While 95 seems a bit much to me, 80px or even less should suffice iirc. #30586)

@blizzz blizzz mentioned this pull request Feb 1, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@blizzz blizzz mentioned this pull request Mar 7, 2023
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
This was referenced May 3, 2023
@blizzz blizzz mentioned this pull request May 17, 2023
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@skjnldsv
Copy link
Member

As there is no feedback since a while I will close this ticket. If this is still happening please make sure to upgrade to the latest version. After that, feel free to reopen.

@skjnldsv skjnldsv closed this Feb 24, 2024
@skjnldsv skjnldsv added bug and removed 3. to review Waiting for reviews labels Feb 24, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 29 milestone Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Footer height in public view too small if imprint/privacy link set
6 participants