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

Migration to html5 and bootstrap4 #1681

Merged
merged 30 commits into from
Feb 19, 2021
Merged

Migration to html5 and bootstrap4 #1681

merged 30 commits into from
Feb 19, 2021

Conversation

nikhiljohn10
Copy link
Contributor

@nikhiljohn10 nikhiljohn10 commented Nov 12, 2020

HTML and Bootstrap updates

NOTE: This update is only for the English language.

  • Updated bootstrap link URLs
  • Updated DOCTYPE as per HTM5 standards
  • Updated CSS for Bootstrap4
  • Added container to page-header
  • Fixed text case inside HTML
  • Updated example year to 2020 in the tutorial
  • Added actions division for alignment
  • Converted Glyphicon to Bootstrap4 Icons

HTML, Bootstrap and Icons have to be updated together. Otherwise, either one will make the page look broken.

This is a followup from #1678

en/django_forms/README.md Outdated Show resolved Hide resolved
@nikhiljohn10
Copy link
Contributor Author

@ekohl I have tested all commits above by doing the updated tutorial from the stagging site https://django.nikz.in/en

image

image

image

There is some additional code I have added to the Django site shown above. The base.html file has a vacant space on the right side when using col-md-8 for layout. I have filled up this vacant space using a bootstrap card.

# base.html
...
        <div class="content container">
            <div class="row">

                <div class="col-md-8">
                  {% block content %}
                  {% endblock %}
                </div>

                <div class="col-md-4">
                    <div class="card">
                      <div class="card-body">
                        <h5 class="card-title">Ads</h5>
                        <p class="card-text">Advertisements goes here</p>
                        <a href="#" class="btn btn-default">Learn more</a>
                      </div>
                    </div>
                </div>

            </div>
        </div>
...

Since this code is not inside the above commit, I would like your suggestion on it. Should I add it and push it into this PR?

Following is the current structure of the blog app:

image

en/django_forms/README.md Outdated Show resolved Hide resolved
en/css/README.md Outdated Show resolved Hide resolved
@ekohl
Copy link
Collaborator

ekohl commented Nov 19, 2020

I don't think we should encourage ads. It's fine to keep it small and empty if it means we don't overwhelm students. Perhaps you can add a side bar in the extension tutorial.

@nikhiljohn10

This comment has been minimized.

@das-g
Copy link
Member

das-g commented Nov 19, 2020

I don't think we should encourage ads. It's fine to keep it small and empty if it means we don't overwhelm students. Perhaps you can add a side bar in the extension tutorial.

@ekohl How about this much code to just fill the blank space?

I like @ekohl's idea of making that a tutorial extension.

The more often we let participants cut and paste and the longer the cut-and-pasted code blocks are, the less likely it is that participants will look at these code carefully, try to understand them and maybe ask their coaches about what those code blocks' purpose is and how the code works. The goal of the Django Girls tutorial is to make programming and software development approachable and to make it seem less like obscure magic. So it shouldn't be like a lengthy and opaque incantation, that has been passed down to you by higher powers and that you have to exactly reproduce to mystically attain its intended effect. Instead, it should provide participants with a glimpse of how software development works (including aspects other than programming, like revision control and deployment) and provide them with a chance of understanding what they're told to do, so that they can become creative themselves.

Therefore, the Django and web development part of the Django Girls tutorial proper should stick to only making a "minimal viable product" for the blog software, and to getting that deployed. (Actually, what the tutorial has the participants currently build, is not even "minimal viable" but just "minimal" in even aspects that are more central than a polished layout with perfectly used screen estate, such as security. E.g., for the sake of brevity, the site's secret key is simply left card-coded and pushed into a public GitHub repo.)

@nikhiljohn10
Copy link
Contributor Author

@das-g That is a valid point. I'll follow that concept with the content.

@ekohl In that light, I modified "col-md-8" to just "col". It will expand the content to 100% width inside container. Otherwise we can make it "col-md-12".

Either way, that vacant space has to go. It troubled my focus when I started django girls tutorial at my learning stage.

# base.html
...
        <div class="content container">
            <div class="row">

                <div class="col">
                  {% block content %}
                  {% endblock %}
                </div>
...

Copy link
Member

@das-g das-g left a comment

Choose a reason for hiding this comment

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

Thank you! Much less overwhelming now, I think.

IMO only minor issues left:

en/django_forms/README.md Outdated Show resolved Hide resolved
en/django_forms/README.md Outdated Show resolved Hide resolved
@nikhiljohn10
Copy link
Contributor Author

@ekohl @das-g All the review changes have been updated.

en/css/README.md Show resolved Hide resolved
en/django_forms/README.md Outdated Show resolved Hide resolved
en/django_forms/README.md Outdated Show resolved Hide resolved
en/django_forms/README.md Outdated Show resolved Hide resolved
nikhiljohn10 and others added 2 commits November 26, 2020 18:41
Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>
Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>
en/django_forms/README.md Outdated Show resolved Hide resolved
en/django_forms/README.md Outdated Show resolved Hide resolved
en/django_forms/README.md Outdated Show resolved Hide resolved
en/django_forms/README.md Outdated Show resolved Hide resolved
nikhiljohn10 and others added 3 commits November 27, 2020 09:25
Co-authored-by: Raphael Das Gupta <github.com@raphael.dasgupta.ch>
Co-authored-by: Raphael Das Gupta <github.com@raphael.dasgupta.ch>
Co-authored-by: Raphael Das Gupta <github.com@raphael.dasgupta.ch>
en/css/README.md Outdated Show resolved Hide resolved
en/django_forms/README.md Outdated Show resolved Hide resolved
@nikhiljohn10
Copy link
Contributor Author

@das-g Is there anything more needed in this PR to merge?

Copy link
Member

@das-g das-g left a comment

Choose a reason for hiding this comment

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

Is there anything more needed in this PR to merge?

I must confess I lost track, but I think it's fine now. What do you say @ekohl?

@nikhiljohn10
Copy link
Contributor Author

Is there anything more needed in this PR to merge?

I must confess I lost track, but I think it's fine now. What do you say @ekohl?

Thanks @das-g. Hi @ekohl, does this PR need any more changes?

@nikhiljohn10
Copy link
Contributor Author

I guess this PR might be falling in to lost and found.

@das-g
Copy link
Member

das-g commented Feb 19, 2021

Eh, I'll merge this as-is.

@das-g das-g merged commit 937d95f into DjangoGirls:master Feb 19, 2021
das-g added a commit to das-g/django-girls-tutorial that referenced this pull request Feb 19, 2021
if no other article elements have been mentioned, yet.
(Previously they were `div` elements and the now-`header` element
preceding them was then also a `div`. But sind DjangoGirls#1681 not anymore.)
das-g added a commit to das-g/django-girls-tutorial that referenced this pull request Feb 19, 2021
if no other article elements have been mentioned, yet.
(Previously they were `div` elements and the now-`header` element
preceding them was then also a `div`. But since DjangoGirls#1681 not anymore.)
ekohl pushed a commit that referenced this pull request Feb 23, 2021
if no other article elements have been mentioned, yet.
(Previously they were `div` elements and the now-`header` element
preceding them was then also a `div`. But since #1681 not anymore.)
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.

3 participants