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

Bootstrap 5 initialization #500

Merged
merged 10 commits into from
Jan 12, 2023
Merged

Conversation

TonisOrmisson
Copy link
Contributor

@TonisOrmisson TonisOrmisson commented Jan 10, 2023

related to #488 & upgrade to bootstrap 5

This is the initial views set with bootstrap 5.

### 1. all resources moved away from src folder

all views-related dependencies as non-mandatory

this allows to choose between bs5/4/3 or anyhing else.

implements bootstrap5 in the "default" views

This is an initial version. Feel free to adjust & comment. Possibly some corners not covered.

Documentation is not updated yet

Also a bug discovered. the 2fa qr-code implementation is not working >=php8.1. I'll post an issue on that

I suggest to merge and call to test and polish the views. I haven't done much bs5 before, so take it as a starting point.

@maxxer
Copy link
Collaborator

maxxer commented Jan 11, 2023

Why not suggesting bs3 views by default? That would allow seamless upgrade.

@maxxer
Copy link
Collaborator

maxxer commented Jan 11, 2023

I was mistaken, I see you don't even suggest the usuario views for bs5 :) Please add them to composer.json

@TonisOrmisson
Copy link
Contributor Author

bs5 views are bundled here still, no separate repo. why not bs3? there would be no PR then left, its pretty much all about bs5.

@maxxer
Copy link
Collaborator

maxxer commented Jan 11, 2023

Sorry, didn't check the views' changes. I remembered the original idea was to abstract and remove views from the repo.

So, basically, now included views are for BS5, but others can be installed and used via viewPath.

Out of curiosity, why moving views outside src/User?

@TonisOrmisson
Copy link
Contributor Author

Yes. The idea is that there is an "official" or "example" set of views that is "supported". It might also be a separate repo (since you anyway need to include some suggested dependencies to get it working), but I did not want to make it somewhere in my namespace etc. So either included in this repo or later create a usuario/bs5-views or similar. Btw I think also usuario/bs3-views repo should be created for the backwards compatibility along this change.

The views are removed from under src folder mainly to indicate that they are optional. But also I think its a good idea and a growing good practice to keep only "code" under src. So also docs could have a separate root folder etc. Btw its also a direction the initial v2 branch here went ( https://github.com/2amigos/yii2-usuario/tree/usuario-2.0-dev ). Makes it a bit easier to filter eg searches, static code testing, code coverage analysis etc to focus only on "code" under src. Currently there should be no bootstrap dependencies in src folder.

@maxxer
Copy link
Collaborator

maxxer commented Jan 11, 2023

Ok, I'll merge.

We need to update i18n config to fetch from this other this as well.

About i18n, we should investigate how to deal with it for "remote" views: bs3 and bs4 files needs to use the same strings, or implement translations in their own package

@TonisOrmisson
Copy link
Contributor Author

forgot the i18n path, final push coming in

@maxxer
Copy link
Collaborator

maxxer commented Jan 11, 2023

resources/i18n/config.php should include the views path as well

@TonisOrmisson
Copy link
Contributor Author

OK, I see we have a problem with the i18n with some string usages being in src, some being outside src since the i18n config for scanning finding the translations can only have one path.

Didn't think about that :) So still a better idea to have the "supported" views under src to be able to manage the translations?

@maxxer
Copy link
Collaborator

maxxer commented Jan 11, 2023

OK, I see we have a problem with the i18n with some string usages being in src, some being outside src since the i18n config for scanning finding the translations can only have one path.

Ye :(

Didn't think about that :) So still a better idea to have the "supported" views under src to be able to manage the translations?

Unless we use two config files and two translation categories, but that's likely to duplicate some efforts for strings which are common in the two areas. I cannot think anything else ATM.

@maxxer maxxer merged commit b8e34e0 into 2amigos:v2.0.0-dev Jan 12, 2023
@TonisOrmisson TonisOrmisson deleted the bootstrap-5-new branch January 12, 2023 13:37
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