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

A better .gitignore and dynamic footer columns based on active footer widgets #776

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Remzi1993
Copy link

@Remzi1993 Remzi1993 commented May 20, 2024

Generated a better .gitignore using gitignore.io and made the footer columns dynamic based on active footer widgets.

Basically checking how many footer widgets are active:

function bootscore_count_active_footer_widgets(): int {
    $count = 0;
    for ($i = 1; $i <= 4; $i++) {
        if (is_active_sidebar('footer-' . $i)) {
            $count++;
        }
    }
    return $count;
}

And then base the columns on that:

function bootscore_get_footer_widget_class(): string {
    $widget_count = bootscore_count_active_footer_widgets();
    return match ($widget_count) {
        1 => 'col-12',
        2 => 'col-6',
        3 => 'col-4',
        default => 'col-3',
    };
}

In .gitignore I removed the package-lock.json because it's intended to be checked into source control. See: https://stackoverflow.com/questions/44206782/do-i-commit-the-package-lock-json-file-created-by-npm-5 Otherwise you might be fighting your team mates when something works on your PC and not on another one. package-lock.json ensures that everyone is using the same versions of those packages.

@Remzi1993 Remzi1993 changed the title A better .gitignore and dynamic footer columns based activated footer widgets A better .gitignore and dynamic footer columns based on active footer widgets May 20, 2024
@crftwrk crftwrk linked an issue May 21, 2024 that may be closed by this pull request
1 task
@crftwrk
Copy link
Member

crftwrk commented May 21, 2024

Hi Remzi,

Thank you for this PR, we're are always happy if someone wants to contribute! But I have some questions.

Why do we need this massive .gitignore? Never seen this before even in large projects.

About the footer. We had already some thoughts on this but we always end up as it is. Because since v6 we have those sleek filters which allows to edit columns in a simple way. Additionally you can use a widget in footer 1, 3, 4 and leave footer 2 empty as a spacer. This would not be possible with dynamic widgets.

Just for upcoming PR's:

  • Please use default 2 empty spaces for formatting instead 4 or tab.
  • Functions should be added in a functions file in folder inc and not in a template file.
  • Please do not pack different types of changes into one PR, split them into two. One for the footer and one for the .gitignore. This helps us to keep track of things and, if necessary, to be able to reverse a PR.

I set the PR to draft so it wouldn't be merged accidentally. Let's wait for @justinkruit for his thoughts.

@crftwrk crftwrk marked this pull request as draft May 21, 2024 17:33
@Remzi1993
Copy link
Author

Remzi1993 commented May 21, 2024

I'm going to answer your questions step by step:

Why do we need this massive .gitignore?

If you look at: https://www.toptal.com/developers/gitignore?templates=linux,macos,windows,composer,wordpress,intellij+all,phpstorm+all,visualstudiocode,node,yarn
You will see I have ignored all operating systems: Windows, Linux and Mac and the 3 populair IDE's (IntelliJ IDEA, PhpStorm and Visual Studio Code), I have also ignored WordPress files, Composer, Node and Yarn.

These are either all the things you use or other devs could use like IntelliJ or VS Code. My advice would be to either generate your .gitignore and include als much as you can then too little. Because you don't want to include things which take up space which are recreated all the time. BTW, I use PhpStorm for this project, but use IntelliJ and VS Code for different things.

But to come back to your question, why? Because multiple people work or in the future might work on you with this project and all those people might use different operating systems with all their junk files, like .ini files on Widnows or DS_Store files on MacOS, do you want those useless files in the repo? I don't think so, same with the 3 populair IDE's people can use, they also have junk files. Node has modules directory which should not be checked in source control and the same with Composer which has the vendor directory.

Also, it couldn't hurt to include a little bit more things than too little, but if you don't like it I suggest to edit and include or remove what you like at: https://www.toptal.com/developers/gitignore?templates=linux,macos,windows,composer,wordpress,intellij+all,phpstorm+all,visualstudiocode,node,yarn

Never seen this before even in large projects.

I have seen this at 1/3 of the projects who do generate it and others only include project specific and don't include IDE's, the biggest .gitignore things are IDE's because they create a lot of junk. They just tell others to be careful what to commit in git. If you leave out the IDE's and Yarn then you will also have a small .gitignore file, but a larger .gitignore file will not harm you in any way.

Additionally you can use a widget in footer 1, 3, 4 and leave footer 2 empty as a spacer. This would not be possible with dynamic widgets.

I think this is possible with my code, you would simply have 2 columns instead of 1. Have you test this out? This code has nothing to do with widgets but only show a number of columns depending how may footer widgets are active (used).

I would give users a choice, now they don't have any choice because there are 4 columns hard coded. What if they want 2 columns? They can if they have a footer based on how many footer widgets are active then the users can decide how many columns the footer is going to have.

Just for upcoming PR's:

  • Please use default 2 empty spaces for formatting instead 4 or tab.
  • Functions should be added in a functions file in folder inc and not in a template file.
  • Please do not pack different types of changes into one PR, split them into two. One for the footer and one for the .gitignore. This helps us to keep track of things and, if necessary, to be able to reverse a PR.

Will keep this in mind, could you please put this in README or a separate CONTRIBUTING file? This would be easier if someone wants to contribute.

@crftwrk
Copy link
Member

crftwrk commented May 22, 2024

Got it with .gitignore and agree. But when we open this box, this must be added to all child and plugin repos as well. This lines can be deleted, because this files have been moved to assets folder:

css/main.css
css/main.map

For the footer widgets, yes its possible to create empty cols but it adds the mb-3 class to it because an empty <div> is recognised as a widget. It is not responsive in your example too. However, as I mentioned something like this we had in mind as well but always end up as it is now. I do not really get warm with it and we should rethink this in v6.1. Focus is now on v6 stable.

Will keep this in mind, could you please put this in README or a separate CONTRIBUTING file? This would be easier if someone wants to contribute.

Absolutely! A CONTRIBUTING.md is something we should add here. At the moment we are struggling with stuff for v6 stable which have a higher priority. If you are able to create one, we will greatly appreciate it.

@Remzi1993
Copy link
Author

yes its possible to create empty cols

My apologies, that's indeed not a nice solution. I did a quick and dirty fix for a client. The columns should only be created if there are active widgets in Footer 1, Footer 2, etc....

I can't look at it at the moment because I need to finish 2 other school projects first. Maybe I can come up with a solution when the academic year ends and when the summer holiday starts.

@justinkruit
Copy link
Member

As response on the .gitignore;

I agree we could add a bit more, like for example the PhpStorm and VSCode files. However, one of the reasons that things from node.js are excluded is because we simply don't work with those in this theme. It's build to be able to run on a PHP web server, without the need for a SSH connection to compile the files. Also generally, when working with PhpStorm, you should already have the project root set to the main WordPress folder instead of the theme (recommended by PhpStorm itself). I also see you added WordPress files to the .gitignore; this is a repo for a theme, so it wouldn't make any sense to include things like wp-config.php etc. If you have those in your repo, you're already setting up your git environment wrong. Same goes for a lot of the Windows, Linux and Mac lines. They are often root folder/files, which is absolutely not where you should have your git repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

[Feature Request] Dynamic footer columns based on active footer widgets
3 participants