Feature 219 styling site landing page #221

ron-huberfeld · 2021-02-06T20:16:29Z

Handle issue #219

Modify base template and home page
Add 404 page for missing pages

…into develop

aviadamar · 2021-02-07T00:25:25Z

app/templates/base.html

+                            <a class="nav-link" href="{{ url_for('not_implemented') }}">Sign Up</a>
+                        </li>
+                        <li class="nav-item">
+                            <a class="nav-link" href="{{ url_for('agenda') }}">Agenda</a>


Agenda is not needs to appear in landing page.

Thanks for the comment.
@yammesicka do you approve removing existing links?

aviadamar · 2021-02-07T00:26:03Z

app/templates/base.html

+                            <a class="nav-link" href="{{ url_for('view_invitations') }}">Invitations</a>
+                        </li>
+                        <li class="nav-item">
+                            <a class="nav-link" href="{{ url_for('search') }}">Search</a>


Search is not needs to appear in landing page.

Thanks for the comment.
@yammesicka do you approve removing existing links?

W/E you decide

aviadamar · 2021-02-07T00:29:17Z

app/templates/base.html

+                <div class="navbar-brand">
+                    <a href="{{ url_for('home') }}" aria-label="Home">
+                        <img class="h-8 w-auto sm:h-10"
+                            src="{{ url_for('static', path='/images/icons/calendar-outline.svg') }}" alt="Logo">


please use the ionicons element and not the svg file.

<ion-icon name="calendar-outline"></ion-icon>

Thanks, sure it will be done

@aviadamar was ionicons checked for performance?
and where are we using popover?

the script is in the head as written ?
about the popover, I have no idea, if you find out let me know.

I modified the scripts to be in the head as stated and it solved the warning in console.
Nevertheless, it stores some items in cache which reduce performance.
I changed the icon in the logo (navbar-brand).
But I didn't manage to find a way to do it in the page's tab icon.
Do you know how to do it in the page's tab icon?

@aviadamar - this change done
Please review

aviadamar · 2021-02-07T00:34:16Z

app/templates/base.html

 <head>
    {% block head %}
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">

    <link href="https://cdn.jsdelivr.net/npm/bootstrap@5.0.0-beta1/dist/css/bootstrap.min.css" rel="stylesheet"
        integrity="sha384-giJF6kkoqNQ00vy+HMDP7azOuL0xtbfIcaT9wjKHr8RbDVddVHyTfAAsrekwKmP1" crossorigin="anonymous">
+    <!-- Tailwind CSS -->
+    <link href="https://unpkg.com/tailwindcss@1.9.5/dist/tailwind.min.css" rel="stylesheet">
+    <link rel="icon" href="{{ url_for('static', path='/images/icons/calendar-outline.svg') }}" />


Link to ionicons and use icons elements and not svg files.

Thanks, sure, your wish is my command :)

@aviadamar - I didn't manage to find a way to do it in the page's tab icon.
Do you know how to do it in the page's tab icon?

aviadamar · 2021-02-07T00:35:33Z

app/templates/base.html

+                            <a class="nav-link" href="{{ url_for('agenda') }}">Agenda</a>
+                        </li>
+                        <li class="nav-item">
+                            <a class="nav-link" href="{{ url_for('view_invitations') }}">Invitations</a>


Im not sure this sections needs to be in landing page.

Thanks for the comment.
@yammesicka do you approve removing existing links?

Whatever you want ^^"

aviadamar · 2021-02-07T00:44:18Z

app/templates/index.html

+            </div>
+        </main>
+    </div>
+    <div class="lg:absolute lg:inset-y-0 lg:right-0 lg:w-1/2">


There are 4 divs that capture only 1 img - maybe it can be simplify ?

Sure, will be done

yammesicka

Great work! I've added few of my insights :)

yammesicka · 2021-02-07T06:25:34Z

app/static/style.css

-  background: -webkit-linear-gradient(to right, #FAFFD1, #A1FFCE);
-  background: linear-gradient(to right, #FAFFD1, #A1FFCE);
-}
+/* body {


Please eliminate commented out code

Thanks for the comment.
I'll change it.

Done, please review

yammesicka · 2021-02-07T06:26:46Z

app/templates/base.html

 <head>
    {% block head %}
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">

    <link href="https://cdn.jsdelivr.net/npm/bootstrap@5.0.0-beta1/dist/css/bootstrap.min.css" rel="stylesheet"
        integrity="sha384-giJF6kkoqNQ00vy+HMDP7azOuL0xtbfIcaT9wjKHr8RbDVddVHyTfAAsrekwKmP1" crossorigin="anonymous">
+    <!-- Tailwind CSS -->
+    <link href="https://unpkg.com/tailwindcss@1.9.5/dist/tailwind.min.css" rel="stylesheet">


We don't use Tailwind in this project.
We won't add it 'cause it will make a mess of a code (Bootstrap + Tailwind mishmash) and will make the page much heavier than it should be. :(

No issue, I'll remove it

By the way, I will remove Bootstrap before removing Tailwind :)

That's almost a valid point, but tailwind is harder to work with if you don't use React/Vue/Angular, especially for new users, and will create a bloated code which will load slower. ^^"

If I have to choose only one of them (and that's the case) - I take Bootstrap

@yammesicka - done - removed tailwind :)

app/main.py

ivarshav · 2021-02-07T17:44:43Z

Could you please add visual screenshots in the description? It could be very helpful

codecov-io · 2021-02-07T18:03:10Z

Codecov Report

Merging #221 (f61ec8c) into develop (856c843) will decrease coverage by 0.04%.
The diff coverage is 75.00%.

@@             Coverage Diff             @@
##           develop     #221      +/-   ##
===========================================
- Coverage    99.06%   99.02%   -0.05%     
===========================================
  Files           48       49       +1     
  Lines         2251     2258       +7     
===========================================
+ Hits          2230     2236       +6     
- Misses          21       22       +1

Impacted Files	Coverage Δ
app/main.py	`96.55% <0.00%> (ø)`
app/routers/four_o_four.py	`85.71% <85.71%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 856c843...f61ec8c. Read the comment docs.

…nto develop

yammesicka

Very good job, Kudos :) As always, added few insights. Please take a look.

yammesicka · 2021-02-09T06:08:37Z

app/locales/base.pot

@@ -8,7 +8,7 @@ msgid ""
 msgstr ""


Can you please delete these files? Wev'e changed them so they'll be created only when pushing to main

@yammesicka - removed base.pot ; anything else to remove?

yammesicka · 2021-02-09T06:09:28Z

app/templates/base.html

+                            <a class="nav-link" href="{{ url_for('agenda') }}">Agenda</a>
+                        </li>
+                        <li class="nav-item">
+                            <a class="nav-link" href="{{ url_for('view_invitations') }}">Invitations</a>


Whatever you want ^^"

yammesicka · 2021-02-09T06:09:48Z

app/templates/base.html

+                            <a class="nav-link" href="{{ url_for('view_invitations') }}">Invitations</a>
+                        </li>
+                        <li class="nav-item">
+                            <a class="nav-link" href="{{ url_for('search') }}">Search</a>


W/E you decide

yammesicka · 2021-02-09T06:10:17Z

app/templates/base.html

+    <script src="https://cdn.jsdelivr.net/npm/bootstrap@5.0.0-beta1/dist/js/bootstrap.bundle.min.js"
+        integrity="sha384-ygbV9kiqUc6oa4msXn9868pTtWMgiQaeYH7/t7LECLbyPA2x65Kgf80OJFdroafW"
+        crossorigin="anonymous"></script>
+    <!-- This bootstrap version is needed here because of the toggler bug in the beta version-->


Great job commenting about it

yammesicka · 2021-02-09T06:10:33Z

app/templates/four_o_four.j2

+{% block content %}
+
+<div class="container">
+    <h2 class="text-4xl tracking-tight leading-10 font-extrabold text-purple-900 


Need to adapt from tailwind

yammesicka · 2021-02-09T06:10:45Z

app/templates/index.html

+{% block content %}
+
+<div class="max-w-screen-xl mx-auto">
+    <div class="relative z-10 pb-8 bg-white sm:pb-16 md:pb-20 lg:max-w-2xl lg:w-full lg:pb-28 xl:pb-32">


Need to adapt from tailwind

…into develop

yammesicka · 2021-02-12T01:40:48Z

app/main.py

-    agenda, calendar, categories, celebrity, currency, dayview,
-    email, event, invitation, profile, search, telegram, whatsapp
+    agenda, calendar, categories, celebrity, currency, dayview, email,
+    event, invitation, profile, search, telegram, whatsapp, four_o_four


Please put in the right place:)

ron-huberfeld added 10 commits February 2, 2021 08:30

feat: add calendar picture and icon

92095ae

feat: styling landing page and add 404 page

5b1eecc

fix: remove commented code

eab932b

fix: 404 page naming convention

1564b70

fix: use ionicons instead of svg

72c30ef

Merge 'develop' changes to resolve conflicts

a1e46d4

Merge 'develop' changes 2

a6de077

Merge branch 'develop' into feature-219-styling-site-landing-page

04101dc

fix: handle flake8 issue

c514590

Merge branch 'develop' of https://github.com/PythonFreeCourse/calendar …

f409729

…into develop

aviadamar reviewed Feb 7, 2021

View reviewed changes

yammesicka reviewed Feb 7, 2021

View reviewed changes

ivarshav reviewed Feb 7, 2021

View reviewed changes

app/main.py Outdated Show resolved Hide resolved

ron-huberfeld and others added 4 commits February 7, 2021 21:17

Apply automatic translateable string changes

30a82b0

Merge branch 'develop' of https://github.com/ron-huberfeld/calendar i…

6679c32

…nto develop

Apply automatic translateable string changes

18f779d

fix merge issue

5cf612a

yammesicka requested changes Feb 9, 2021

View reviewed changes

ron-huberfeld added 10 commits February 10, 2021 08:55

Merge 'develop' changes

f9e29ec

Merge branch 'develop' of https://github.com/PythonFreeCourse/calendar …

68a2706

…into develop

Merge branch 'develop' of https://github.com/PythonFreeCourse/calendar …

51305a9

…into develop

Merge 'develop' changes

8c6a35c

Merge branch 'develop' of https://github.com/PythonFreeCourse/calendar …

d07eb44

…into develop

fix: remove tailwind

603523e

fix: remove comments

627ede5

Merge 'develop' changes

5bf93be

fix: remove genereted file

2b8e0d3

fix: move style from tailwind to bootstrap

be9216b

ron-huberfeld added 3 commits February 11, 2021 15:48

fix: remove tailwind related style

b096fbe

fix: reorder routes

2b4e06b

fix: reorder again :)

f61ec8c

ron-huberfeld requested a review from yammesicka February 11, 2021 21:02

yammesicka requested changes Feb 12, 2021

View reviewed changes

ron-huberfeld requested a review from yammesicka February 12, 2021 01:52

yammesicka approved these changes Feb 13, 2021

View reviewed changes

yammesicka merged commit 9bf6657 into PythonFreeCourse:develop Feb 13, 2021

ron-huberfeld deleted the feature-219-styling-site-landing-page branch February 13, 2021 09:07

Feature 219 styling site landing page #221

Feature 219 styling site landing page #221

Conversation

ron-huberfeld commented Feb 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aviadamar Feb 7, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yammesicka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yammesicka Feb 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivarshav commented Feb 7, 2021

codecov-io commented Feb 7, 2021 • edited

Codecov Report

yammesicka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aviadamar Feb 7, 2021 •

edited

yammesicka Feb 9, 2021 •

edited

codecov-io commented Feb 7, 2021 •

edited