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

Ported site to Bootstrap 4 from 3 #347

Merged
merged 18 commits into from
Jan 14, 2022
Merged

Ported site to Bootstrap 4 from 3 #347

merged 18 commits into from
Jan 14, 2022

Conversation

S-kus
Copy link
Contributor

@S-kus S-kus commented Jan 12, 2022

Fixes: #316
I Ported the site to Bootstrap 4 from 3, As shown in this Video, Using this.

Process:

  1. I changed the old "bootstrap.css" and "bootstrap.js" of bootstrap 3 to 4 by using official Documents.
  2. Changed the first-header in nav.html as its links and navbar-toggler were completely broken, and updated the collapse navbar-toggler with previously using hamburger navigation for mobile view.
  3. Major change here is because Flexbox is enabled by default in the v4 version, so I used class="d-flex justify-content-center" for the required places.
  4. Updated the version in the license file.
  5. Updated the airspace.css for some sections to fix the flexbox problem.

@quozl
Copy link
Contributor

quozl commented Jan 12, 2022

Thanks. I did git fetch origin pull/347/head, checked out 4317d5e, looked at Files changed tab in pull request, and compared against Bootstrap release. I did not run the site. Technical review of changes;

  • several places you have commented lines rather than remove them; please remove them,
  • a few changes seem unrelated to the porting, such as the changed Wiki link for Sugar on a Stick, and this is because you have commits from Updated SOAS hyperlink and added Trisquel #344 mixed in; did you start from master or from your existing branch?
  • you've changed bootstrap.css from the release; the changes are very substantial because of indentation; these should be minimal changes instead, or moved to a different file so that we keep the original code unchanged, (part of security auditing),
  • the commit messages don't explain what was done and why; see our guide to writing commit messages.

Also, bootstrap.js is unchanged, and this is good.

@S-kus
Copy link
Contributor Author

S-kus commented Jan 12, 2022

  • several places you have commented lines rather than remove them; please remove them,

It's corrected Now.

  • a few changes seem unrelated to the porting, such as the changed Wiki link for Sugar on a Stick, and this is because you have commits from Updated SOAS hyperlink and added Trisquel #344 mixed in; did you start from master or from your existing branch?

Sorry for this mistake, I fixed it as in the master branch.

  • you've changed bootstrap.css from the release;

Sorry, Sir, I fixed it now similar to the original file.

  • the changes are very substantial because of indentation; these should be minimal changes instead, or moved to a different file so that we keep the original code unchanged, (part of security auditing)

I didn't get this. The actual original bootstrap file is already indented. Could you please clarify once again, Do I need to change the file name or place?

Yes, Sir, I am commenting again with a summary and proper explanation.

@S-kus
Copy link
Contributor Author

S-kus commented Jan 12, 2022

I Ported the site to Bootstrap 4 from 3 using official Document, As shown in this Video, Major change in v4 from v3 is because Flexbox is enabled by default in the v4 version.
Fixes: #316

  • Firstly I updated the old "bootstrap.css" and "bootstrap.js" of bootstrap 3 to 4 by using official Documents and updated the referred file links in base.html.

  • Because of "default flexbox", contents are shifted towards the left. As shown:
    x
    To fix this I used class="d-flex justify-content-center" of bootsrapv4 according to this with class="row".

  • Because of flex enabled and toggle button changed, NavBar links are broken, They are using now default padding, To fix this I use this and updated the li by adding the class "nav-item" and similarly to a with class "nav-link".

  • The Drop-down button is changed now as, So I updated that too with a new "dropdown-toggle" and added class to their item "dropdown-menu" as mentioned in the Navbar section.

  • In the footer after flexbox enabled the referred icons are aligned left and the footer header is no more responsive, To fix this I firstly use "justify-content" for them and shifted the social icons in middle with "contribute to website" div, for better UI and to fit-content properly.

  • As the nav-toggler button is not aligned properly in phone view, as well as earlier mobile view is also not responsive correctly. As shown:
    Screenshot from 2022-01-11 00-51-47
    Screenshot from 2022-01-11 00-51-55
    I updated it to the hamburger button as discussed in Port site to Bootstrap 5 from 4 #316 (comment). For this, I changed the @media in "airspace.css".

  • I changed the required CSS background classes to make things look like earlier on the website. exa: background of dropdown when it's active.

  • Because of nav changes "search bar" is not opened properly to its full length to fix this I removed some of its CSS lines and added classes of bootstrap to align it properly. exa: removed "bottom:1px;".

  • In "sugar-for-raspbian.html" badges are not aligned properly because of flex-enabled. So I added a class of margin-top and bottom for better spacing and UI.

about-us.html Show resolved Hide resolved
@quozl
Copy link
Contributor

quozl commented Jan 13, 2022

@S-kus wrote:

I didn't get this. The actual original bootstrap file is already indented. Could you please clarify once again, Do I need to change the file name or place?

Your copy of the file bootstrap.css looked vastly different to the release version. I downloaded the release version and compared with your latest d5ece68. The release version md5sum is d59729439a203fc474f5677b8d18d8bb and yours is dc08100b074756a5acf45702c0545709. Most of the difference is in indentation, which implies that you took the release version, used a tool that changed the indentation, and then saved it. The remaining difference is very minor, but is important to highlight in the change;

--- bootstrap-4.0.0-dist/css/bootstrap.css 2018-01-19 03:33:20.000000000 +1100
+++ css/bootstrap.css   2022-01-13 11:00:23.332623717 +1100
@@ -4023,8 +4023,9 @@
   font-size: 1.25rem;
   line-height: 1;
   background-color: transparent;
-  border: 1px solid transparent;
+    border: 1px solid white;
   border-radius: 0.25rem;
+    float: right !important;
 }
 
 .navbar-toggler:hover, .navbar-toggler:focus {
@@ -8972,4 +8973,3 @@
     border: 1px solid #ddd !important;
   }
 }
-/*# sourceMappingURL=bootstrap.css.map */

This was generated with diff -wu to ignore whitespace.

@quozl
Copy link
Contributor

quozl commented Jan 13, 2022

@S-kus wrote:

Yes, Sir, I am commenting again with a summary and proper explanation.

Sorry, I was talking about the commit messages, not the pull request comments. Pull requests are disposable; they are lost over time, are not kept in git, and especially they would be lost if we shifted to another git hosting solution. Commit messages are kept in git. We use them during git bisect.

If you don't have the skills to rewrite commit messages, just say so, and I'll rewrite them for you. I was about to do this to save time, but found I had more questions for you. You may be interested in our Guide for reviewers to see what our goals are.

p.s. please don't call me sir, it is not respectful in my egalitarian culture.

@S-kus
Copy link
Contributor Author

S-kus commented Jan 13, 2022

@quozl wrote:

Your copy of the file bootstrap.css looked vastly different to the release version. I downloaded the release version and compared with your latest d5ece68. The release version md5sum is d59729439a203fc474f5677b8d18d8bb and yours is dc08100b074756a5acf45702c0545709. Most of the difference is in indentation, which implies that you took the release version, used a tool that changed the indentation, and then saved it. The remaining difference is very minor, but is important to highlight in the change;

--- bootstrap-4.0.0-dist/css/bootstrap.css 2018-01-19 03:33:20.000000000 +1100
+++ css/bootstrap.css   2022-01-13 11:00:23.332623717 +1100
@@ -4023,8 +4023,9 @@
   font-size: 1.25rem;
   line-height: 1;
   background-color: transparent;
-  border: 1px solid transparent;
+    border: 1px solid white;
   border-radius: 0.25rem;
+    float: right !important;
 }
 
 .navbar-toggler:hover, .navbar-toggler:focus {
@@ -8972,4 +8973,3 @@
     border: 1px solid #ddd !important;
   }
 }
-/*# sourceMappingURL=bootstrap.css.map */

This was generated with diff -wu to ignore whitespace.

I got it, The indentation is probably because of my editor and mistake, I corrected it now.

@S-kus
Copy link
Contributor Author

S-kus commented Jan 13, 2022

I was talking about the commit messages

Sorry, I mistakenly thought it was comments and proper description. I can do that, Could you tell me? Should I revert the earlier commits and do new commits sequentially like steps. I will be careful with such mistakes next time also.

@quozl
Copy link
Contributor

quozl commented Jan 13, 2022

Yes, I'm happy to explain how to do that. What tools do you use for making the git commits? I probably use different tools, so I have to translate a bit.

@S-kus
Copy link
Contributor Author

S-kus commented Jan 13, 2022

@quozl I use the Linux terminal.

@quozl
Copy link
Contributor

quozl commented Jan 13, 2022

Okay, I use that, as well as emacs with magit. There are so many ways to rewrite commits, and everybody has a different preference. People also have different names for git remotes. Here's one way to rewrite;

  • update your repository with the latest upstream commits; git fetch --all
  • start a new branch so as to avoid impact to this branch; git checkout -b migratev4-rebase
  • reset to the latest origin/master, which is currently 29871ab ("Update Gemfile.lock"); git reset 29871ab

At this point, your working tree will contain the changes you have made, but the repository branch will be back in time, which gives you a chance to make your commits again.

  • use git diff to see what is yet to be committed,
  • use git add to add files, or git add --interactive if you'd like to choose parts of the change,
  • use git commit to join a collection of changes together into a commit that can be explained; look at your large description above for ideas on how to split the work up,
  • use gitk --all or another visualisation to self review your change,

Iterate until you are happy with the new branch. At any time you can reset again. Once you are happy, note the git hash git log -1, then switch to the branch of this pull request git checkout migratev4 and reset this branch to the same git hash. git reset --hard ...

Finally, once your local branch on your computer looks okay, you can git push --force to update this pull request with it.

If you've never done anything like this before, it may help your comfort level by making a backup of your repository first.

@S-kus
Copy link
Contributor Author

S-kus commented Jan 13, 2022

@quozl I done the required changes and updated the commits in steps. Please review it.

@quozl quozl merged commit 446c389 into sugarlabs:master Jan 14, 2022
@chimosky
Copy link
Member

Thanks for working on this @S-kus.

@S-kus
Copy link
Contributor Author

S-kus commented Jan 14, 2022

I am going through the site again, Before my commits, the last pr which merged was about the profiles page. i.e. #340. for the styling of this page separate CSS file i.e. contributors.css used. Which creates some problems with the footer and navbar styling (specifically nav-bar "li"). As shown:
Screenshot from 2022-01-14 19-02-24
Should I create a new issue and raise pr for this?
@quozl @chimosky what are your opinions about this?

@quozl
Copy link
Contributor

quozl commented Jan 14, 2022

I reproduce dots in the navbar.

Anyone may fix anything. Yes, please proceed.

We ought also alert @nikkuAg in case this is a regression introduced by 1ae8e43.

I don't mind who fixes it first. We don't like to get into allocation of work, because the effort to do allocation is almost always futile.

Always check master branch hasn't changed before starting work on a fix, in case someone else fixed it. Sometimes, for urgent fixes, we may just push commits rather than make a pull request.

@S-kus
Copy link
Contributor Author

S-kus commented Jan 15, 2022

Okay, @quozl. I am working on it.

Other than this page is not responsive. As shown,
Screenshot from 2022-01-15 11-42-12

@quozl
Copy link
Contributor

quozl commented Jan 15, 2022

Okay, no worries. By the way, you do not need to create an issue unless you decide to abandon working on it.

@chimosky
Copy link
Member

@quozl @chimosky what are your opinions about this?

I agree with what quozl has said.

@S-kus
Copy link
Contributor Author

S-kus commented Jan 15, 2022

I fixed the problem in this. As shown in this Video.
@quozl @chimosky Please review it.
Should I raise a new pull request for this?

@quozl
Copy link
Contributor

quozl commented Jan 15, 2022

Thanks for making a video, but I'll wait to review a pull request. You don't need our permission to make a pull request, you can do it at any time you have a change to make.

@S-kus S-kus mentioned this pull request Jan 15, 2022
@S-kus
Copy link
Contributor Author

S-kus commented Jan 15, 2022

@quozl I done the required changes and created the pull request. Please review it.

@S-kus S-kus mentioned this pull request Jan 26, 2022
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.

Port site to Bootstrap 5 from 4
3 participants