-
Notifications
You must be signed in to change notification settings - Fork 90
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
Upgrade to Bootstrap 4 #332
Conversation
It looks like the build is failing? Also, this diff is massive! I think that the themes didn't previously require as much CSS to be present in this repo. I think it was downloaded during the build process. Is it possible for us to revert to that setup? |
5a55535
to
fa524f0
Compare
I fixed the sass-lint issues
Oh, yeah, I have actually missed that. In fact it looks like we don't need the themes CSS to be present in the repo at all, it gets downloaded anyway. |
I think the themes are required (not the full CSS) since they contained app-specific color overrides. Can you take a look at the old versions? |
assets/css/general.css
Outdated
/* Utilities */ | ||
|
||
/* sass-lint:disable class-name-format */ | ||
.position-relative { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does BS4 not already come with an equivalent? I was under the impression that a lot of these utilities could be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the unnecessary ones in latest push
No need to squash commits. I'll do that on merge. |
Oh, I understand the themes now. So they are overwrites if we build specifically for a theme. I could revert them, the problem might be that I don't think that 3.x themes are compatible with Bootstrap 4 and the new 4.x themes have some new ones and some old ones are missing. |
The overrides are just for app-specific stuff though. They're not dependent on bs3 and if they are, it's only a couple lines to change? |
Yeah, but the set of themes for v3 isn't identical to the set for v4. E.g. theme called 'paper' no longer exists, I think it got replaced by 'materia' and I'm not sure if they have the same look. Also, there are a few new themes, which weren't there before. I'm not sure what to do with those. I'll definitely revert the themes folder and do a bit more research. If there were just renames, I'll try to rename the overwrites where applicable and I think it would be good to open another issue to add overwrites for new themes where needed. How about that? |
Ohhh. Don't worry about old themes. For new themes, just add similar overrides. I'm pretty sure the overrides just copy over some colors. |
Ok, thanks! I'll do it tomorrow when I'm back from school. Could you also have a look at the site, please? It should be good (excluding the themes) but there were subtle changes, mostly related to how Bootstrap handles spacing now. I don't think they're breaking changes and actually they might even be better for usability on mobile phones so I left them with defaults. It's a few pixels of a difference compared to current master. |
Took a look at the translator interface and it's pretty close! There are however a couple of mostly minor issues. I'll take a look at the other interfaces after this one is fixed :)
|
The issues you mentioned, including themes should be fixed now. |
|
Thank you for the additional time! I have fixed the first two issues and also adjusted the vertical spacing on the checkboxes. But could you also guide me how to enable those beta changes on the local build? I can't get the navbar entries that you were talking about, even when building from master. Am I missing some special make command? |
@myfrom You can enable the navbar and more modes through the config.conf file. |
I recently left the beta config on another issue, the one about the 404 warning being misplaced. |
#329 specifically |
Don't worry about adding extra non-BS styles. If this is Bootstrap's new styling for the pill component, so be it. |
199d3ff
to
f9783ac
Compare
Fixed. The styling of nav element has changed visibly in Bootstrap 4 - especially if you look at the top navigation (from beta) - but as per your previous comment, I left it unchanged. |
Some minor concerns before I think this is good for task approval. I'm not necessarily going to merge it yet because there are some more minor tweaks that I think it'll need but that can be another task (feel free to take it up).
|
If it's pretty involved, we can leave it for another task/issue since I think you've done a fair bit of work on this already. |
I haven't been able to test with this set of languages but if I put in there random text it overflows on live version too. I added text ellipsis in my edits.
I removed Bootstrap's
I changed it up a bit so it should be nicer now. I don't want to use floats here because of a few things related to layout that Bootstrap 4 changed so I manipulate bottom padding of the navbar depending on the size of the device. Should behave similarly to original but I can't guarantee that on every locale. On the RTL version, it should be fixable with a few additional styles. I think it shouldn't take too long. I'll start working on that. |
It seems to wrap here which is better IMO:
Sounds good!
I wouldn't spend more than 15 minutes on it overall. Like I said, I just want the broad strokes to be resolved here.
Awesome! |
Ah, looks good! |
RTL should be fine now. There's a bug with language selection dropdowns but it's probably related to JavaScript, not the styles and it also appears on live version. |
I'm going to accept this task since I think you've done a great job with it. There are things that need to be cleaned up for a merge of this as v4 of the app. If you're interested, let me know and I can create a new task. e.g. the alignment here There are some other inconsistencies such as some extra stuff being enabled in mobile that doesn't fully work on mobile. The one thing I do request is for the random double semicolon and a description of how you generated this bootstrap.css file so that it can be duplicated or something similar in the future. |
Co-Authored-By: myfrom <[email protected]>
Also, what is this bug you're referring to? |
Thank you! I've used this tool to build the theme. I didn't think of exporting it as I was talking about a bug that makes all the languages go into one column in RTL locale: |
Awesome, thanks!
Huh, nice find. Do you mind creating an issue? |
Here's the file: And I opened an issue for that bug - it's #333 |
@myfrom - awesome! lmk if you want me to make a task that involves cleaning up the rest of the details here. |
So what bit works are not finished? |
I honestly don't remember. You'd have to try it. If you don't see anything that doesn't match up with the current site, I can take a look. |
I cant find any changes except size differents. May be im not familiar with it. Can you please take a look at it? |
@sushain97 Can you please. :) |
There are actually a bunch of minor things mentioned in this thread. |
@sushain97 After a analysis in this pull and testing, These are my output but still i cant find which isnt fixed yet. :(
fixed
Fixed.
FIxed.
Fixed.
Fixed.
Fixed.
Fixed.
Fixed.
Fixed.
Fixed.
Fixed.
Its talked not needed. |
Did all the necessary changes to upgrade the web app to Bootstrap 4. As far as I tested, everything should work fine.
Fixes #200