created gallery page issue #213 #227

coding-geek21 · 2021-04-08T16:33:54Z

Related Issue

Fixes: #213

Prosposed Changes

change 1
change 2

Additional Info

any additional information or context

Screenshots

Original	Updated
original screenshot

Jagrati-Bot · 2021-04-08T16:34:14Z

Congratulations for making your first Pull Request at JagratiWebApp!! 🎉 Someone from our team will review it soon.

garg3133 · 2021-04-09T12:09:58Z

@coding-geek21 On the first look:

Don't keep the background as orange, it looks odd. Keep the background simply white or some light grey shade.
Name the page as "Captures".
Instead of putting the events links at the top, make different sections for each event. Like:

Event 1

Photos...

Event 2

Photos...
You do not need to add the libraries which are already present in layout.js (for example, Bootstrap and JQuery).

Will give you more extensive review on the code once the page looks as we expect it to look! Thanks for working on this!

coding-geek21 · 2021-04-09T12:18:25Z

I will make the necessary changes thanks for the review :)

…bApp into branch01

garg3133 · 2021-04-12T11:29:35Z

Hey @coding-geek21, please also leave a comment whenever you make some updates in the PR (so that we can receive a notification) along with the screenshot of the new page (so that we can give you some feedback right away without pulling the PR on our systems and testing it). This will also help you in getting the reviews faster.

coding-geek21 · 2021-04-12T11:45:16Z

@garg3133 Sorry, actually I thought of showing demo, I forgot it . I will do it now

coding-geek21 · 2021-04-12T11:57:20Z

12.04.2021_17.22.44_REC.mp4

@garg3133 Please review this Thanks!

garg3133 · 2021-04-12T13:55:16Z

This looks pretty great @coding-geek21!

A few suggestions:

For the main page heading, make it all capital, remove the underline and increase the font weight. Also, add an ❤️ after Captures (CAPTURES ❤️).
Change the name to Captures only on the homepage navbar and remove the "Alumni" link from the navbar.
Keep the colour of event name black only and align it to the left instead of centre.
Use the images present in static/home/images only (as we will be anyways fetching the images from the database only).

home/urls.py

home/views.py

templates/gallery_page.html

garg3133 · 2021-04-12T13:58:39Z

templates/gallery_page.html

+{% block style %}
+
+<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/twitter-bootstrap/4.6.0/css/bootstrap.min.css" integrity="sha512-P5MgMn1jBN01asBgU0z60Qk4QxiXo86+wlFahKrsQf37c9cro517WzVSPPV1tDKzhku2iJ2FVgL67wG03SGnNA==" crossorigin="anonymous" />
+<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.6.0/jquery.min.js" integrity="sha512-894YE6QWD5I59HgZOGReFYm4dnWc1Qt5NtvYSaNcOP+u1T9qYdvdihz0PPSiiqn/+/3e7Jo4EaG7TubfWGUrMQ==" crossorigin="anonymous"></script>


I think bootstrap and jquery is already being used in layout.html, so you don't need to add these here.

templates/gallery_page.html

coding-geek21 · 2021-04-12T14:13:47Z

Thanks for the review!

I will make the changes and give updates.

coding-geek21 · 2021-04-12T16:10:53Z

@garg3133 Thank you for your detailed feedback! I made all the requested changes please review it . And I tried running the page without having the jquery links that you mentioned already there in layout.html , but it's not working.

12.04.2021_21.33.25_REC.mp4

garg3133

And I tried running the page without having the jquery links that you mentioned already there in layout.html , but it's not working.

I see lightbox is using the jQuery you added here. But you can still remove the bootstrap link as it is mostly redundant.

Also, can you format the HTML file? Use Alt + Shift + F if using VSCode (also verify if everything is working fine after the changes).

Apart from this and a few reviews mentioned below (which are mostly meant to make the code cleaner and easier to read), the PR mostly looks good now.

templates/home/index.html

garg3133 · 2021-04-13T10:08:18Z

templates/home/captures.html

+                style="color: red"></i></h1>
+        </div>
+
+        <br><br>


Instead of br tags, you should always use margins for adding vertical spaces.

Note: Do not use <br> to create margins between paragraphs; wrap them in <p> elements and use the CSS margin property to control their size.

Source: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/br

templates/home/captures.html

garg3133 · 2021-04-13T10:27:41Z

templates/home/captures.html

+    .captures-page-heading{
+        font-size:35px;
+        font-weight:bolder;
+    }


You can remove font-size from here as it doesn't seem to do anything anyways (h1's default font size overrides this, which is infact well-sized. And instead of font-weight, you can use font-weight-bold class from bootstrap.

templates/home/captures.html

coding-geek21 · 2021-04-13T12:07:57Z

@garg3133 made all the changes and also formatted the code in captures page, I tried running without the links but the lightbox is not working without those links , maybe I guess it's because of different version in layout.html

Please review the changes , Thank you!

garg3133 · 2021-04-13T12:21:05Z

@coding-geek21 You forgot to remove the class names from HTML.

Also, I forgot to mention this before, can you shift the scripts to the {% block scripts %} at the bottom of the template?

Use of .home-btn?

coding-geek21 · 2021-04-13T12:39:48Z

@garg3133 made changes removed classes and .home-btn is of no use I forgot to remove it, also moved the scripts inside the block

Please review it, Thank you!

garg3133 · 2021-04-14T13:32:12Z

Looks good now @coding-geek21! Although the scripts tags in the style block should also be present in the scripts block, I've changed that myself.

Merging it into master 🎉

Thanks for your contribution 🚀

garg3133 · 2021-04-14T13:36:17Z

Would you like to work ahead on this page (which is to fetch the data from the Gallery model of the events app), if you have some familiarity with Django?

Other than that, you can also work on our other pages, like we need to create a page for displaying all the feedbacks we receive in the feedbacks app.

coding-geek21 · 2021-04-14T13:52:41Z

Would you like to work ahead on this page (which is to fetch the data from the Gallery model of the events app), if you have some familiarity with Django?

Other than that, you can also work on our other pages, like we need to create a page for displaying all the feedbacks we receive in the feedbacks app.

@garg3133 yeah sure I would be happy to work on these issues

garg3133 · 2021-04-14T17:54:49Z

@coding-geek21 What would you like? Working ahead on this page itself or on issues like #125, #133, #137, etc. You may check out other issues too and comment on them if you'd like to work on them (even if they're assigned because most of the issues are inactive).

(I was talking about #137 in the earlier comment)

coding-geek21 · 2021-04-15T05:17:33Z

@garg3133 I would like to go ahead on this page for now and I will also look into other issues too

garg3133 · 2021-04-15T11:20:46Z

@coding-geek21 I've created issue #238 for the work on this page (you need to comment on the issue to get it assigned to yourself).

Along with this, I'm also assigning you the issue #137, you can work on both these issues parallelly (first start to work on #238 and then you can start working on #137 until the former is reviewed).

coding-geek21 · 2021-04-15T11:26:51Z

@coding-geek21 I've created issue #238 for the work on this page (you need to comment on the issue to get it assigned to yourself).

Along with this, I'm also assigning you the issue #137, you can work on both these issues parallelly (first start to work on #238 and then you can start working on #137 until the former is reviewed).

Yes sure , Thank you :)

created gallery page

2136530

coding-geek21 added 2 commits April 8, 2021 22:08

Update urls.py

26d6cd7

Update urls.py

b11261c

Jayapritha21 added 2 commits April 11, 2021 17:21

made changes to gallery page

d89447c

Merge branch 'branch01' of https://github.com/coding-geek21/JagratiWe…

76dcffb

…bApp into branch01

garg3133 reviewed Apr 12, 2021

View reviewed changes

made changes to captures page

4f077ae

coding-geek21 requested a review from garg3133 April 13, 2021 09:45

garg3133 reviewed Apr 13, 2021

View reviewed changes

formatted the code and made minor changes

604253b

coding-geek21 requested a review from garg3133 April 13, 2021 12:10

made minor changes

a95c6f7

garg3133 merged commit 6c6c425 into garg3133:master Apr 14, 2021

garg3133 added gssoc21 Level3 labels Apr 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

created gallery page issue #213 #227

created gallery page issue #213 #227

coding-geek21 commented Apr 8, 2021 •

edited

Loading

Jagrati-Bot commented Apr 8, 2021

garg3133 commented Apr 9, 2021 •

edited

Loading

Event 1

Event 2

coding-geek21 commented Apr 9, 2021

garg3133 commented Apr 12, 2021

coding-geek21 commented Apr 12, 2021

coding-geek21 commented Apr 12, 2021

garg3133 commented Apr 12, 2021

garg3133 Apr 12, 2021

coding-geek21 commented Apr 12, 2021

coding-geek21 commented Apr 12, 2021 •

edited

Loading

garg3133 left a comment

garg3133 Apr 13, 2021

garg3133 Apr 13, 2021

coding-geek21 commented Apr 13, 2021 •

edited

Loading

garg3133 commented Apr 13, 2021 •

edited

Loading

coding-geek21 commented Apr 13, 2021

garg3133 commented Apr 14, 2021

garg3133 commented Apr 14, 2021

coding-geek21 commented Apr 14, 2021

garg3133 commented Apr 14, 2021

coding-geek21 commented Apr 15, 2021

garg3133 commented Apr 15, 2021 •

edited

Loading

coding-geek21 commented Apr 15, 2021

created gallery page issue #213 #227

created gallery page issue #213 #227

Conversation

coding-geek21 commented Apr 8, 2021 • edited Loading

Related Issue

Prosposed Changes

Additional Info

Screenshots

Jagrati-Bot commented Apr 8, 2021

garg3133 commented Apr 9, 2021 • edited Loading

Event 1

Event 2

coding-geek21 commented Apr 9, 2021

garg3133 commented Apr 12, 2021

coding-geek21 commented Apr 12, 2021

coding-geek21 commented Apr 12, 2021

garg3133 commented Apr 12, 2021

garg3133 Apr 12, 2021

Choose a reason for hiding this comment

coding-geek21 commented Apr 12, 2021

coding-geek21 commented Apr 12, 2021 • edited Loading

garg3133 left a comment

Choose a reason for hiding this comment

garg3133 Apr 13, 2021

Choose a reason for hiding this comment

garg3133 Apr 13, 2021

Choose a reason for hiding this comment

coding-geek21 commented Apr 13, 2021 • edited Loading

garg3133 commented Apr 13, 2021 • edited Loading

coding-geek21 commented Apr 13, 2021

garg3133 commented Apr 14, 2021

garg3133 commented Apr 14, 2021

coding-geek21 commented Apr 14, 2021

garg3133 commented Apr 14, 2021

coding-geek21 commented Apr 15, 2021

garg3133 commented Apr 15, 2021 • edited Loading

coding-geek21 commented Apr 15, 2021

coding-geek21 commented Apr 8, 2021 •

edited

Loading

garg3133 commented Apr 9, 2021 •

edited

Loading

coding-geek21 commented Apr 12, 2021 •

edited

Loading

coding-geek21 commented Apr 13, 2021 •

edited

Loading

garg3133 commented Apr 13, 2021 •

edited

Loading

garg3133 commented Apr 15, 2021 •

edited

Loading