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

Add extension submodules #55

Merged

Conversation

JonasUJ
Copy link
Contributor

@JonasUJ JonasUJ commented Aug 4, 2020

Further work on #42
Closes #7

I didn't include some extensions because they were extremely out of date or simple not worth the time. Most of the extensions are made by the same guy, so their directory structure all match up, but others have wildly differing setups. I tried my best to have it figure out what files are relevant, meaning adding new extensions should only be a matter of running:

git submodule add <repo-link>
git mv ./[extension_name]/ ./django_simple_bulma/extensions/[extension_name]

Since i removed the old extensions, this is definitely not backwards compatible, because of this I suggest that when this branch is merged we release it as v2 or at least v1.4. Also, the way I've been testing is with one big file with all the extensions, which is only a visual verification. I also haven't tested the custom variables some of the extensions use.

@ghost ghost added the needs 2 approvals label Aug 4, 2020
Copy link
Owner

@lemonsaurus lemonsaurus left a comment

Choose a reason for hiding this comment

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

I didn't include some extensions because they were extremely out of date or simple not worth the time

Which ones? Maybe we should update the Readme with information about which extensions are included and which are not?

this is definitely not backwards compatible, because of this I suggest that when this branch is merged we release it as v2

I agree. That was going to be my suggestion, too.

I also haven't tested the custom variables some of the extensions use.

That's okay, this was never tested before the initial v1.0 release either. If someone runs into a problem with them, they can open a bug report.

adding new extensions should only be a matter of running

Wouldn't you also have to change the MANIFEST.in file? I'm wondering if maybe we should document how to add these additional submodules in the wiki or something.

Overall this looks great. Nice work, and thanks for the PR. I just have a couple of nitpicks and one suggestion for a feature removal.

django_simple_bulma/utils.py Outdated Show resolved Hide resolved
django_simple_bulma/utils.py Outdated Show resolved Hide resolved
Co-authored-by: Leon Sandøy <leon.haland@gmail.com>
@JonasUJ
Copy link
Contributor Author

JonasUJ commented Aug 12, 2020

Wouldn't you also have to change the MANIFEST.in file? I'm wondering if maybe we should document how to add these additional submodules in the wiki or something.

That depends on the extension. Out of 19 extensions I only needed a special case for two in the MANIFEST.in.

Which ones?

Some where duplicates (like steps and tags-input), another is the iconpicker that doesn't even work on the website. The dashboard and blocklist ones are also excluded, I think i had some trouble making them work. The spacing extension became obsolete in Bulma v0.9.0.

@JonasUJ
Copy link
Contributor Author

JonasUJ commented Aug 12, 2020

The readme currently says we use Bulma-Extensions v4.0.0, but I'm unsure of where this number comes from, considering each extension has its own version control.

@lemonsaurus
Copy link
Owner

lemonsaurus commented Aug 12, 2020

it may have been the npm package number at the time.

image

Seems that's at 6.2.4 now.

Would be better to just include a list of which specific extensions we do support, though. That's probably the final thing I'd like to see in this PR before merging. And obviously version isn't really erlevant anymore now with the submodules. It'll be whatever was the latest version when the last release happened. We can do releases now and then to bump these versions.

@JonasUJ
Copy link
Contributor Author

JonasUJ commented Aug 12, 2020

I remember now that the iconpicker extension doesn't work because it uses a fontawesome cdn for version 4 that now responds with 404, probably because it is no longer supported.

@lemonsaurus lemonsaurus merged commit 4681e59 into lemonsaurus:submodule_refactor Aug 12, 2020
@lemonsaurus
Copy link
Owner

excellent!

This was referenced Aug 13, 2020
@JonasUJ JonasUJ deleted the submodule_refactor branch September 10, 2020 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants