Improvements for the Default theme #282

NeaMitika · 2020-06-08T20:06:24Z

I am the author of this code or the code is public domain
I release this code into the public domain

I have done some improvements for the default theme which i think it should be called Light (cuz i am planning to make a Dark one too)...

Some how there is the login.php commit here too... i am pretty new on github i dont really know how to use it...

I have made some modifications to HTMl so it can be more user-friendly.

Improvements to the HTML of the list of episodes

NeaMitika · 2020-06-08T20:21:52Z

I forgot to mention that i have added a custom.css file inside style folder with only a class in it
audio { margin-top: -12px; margin-bottom: -18px; margin-left: 4px; width: -webkit-fill-available; }
and i have added font-awesome - 4.7.0 with a CDN (only used it for the calendar icon near the published date but i think it might me usefull in the future)

emilengler · 2020-06-10T07:08:21Z

Thanks, I will review the PR the next days. I'm currently a bit busy

emilengler · 2020-06-14T10:39:50Z

Sorry, that I am still delaying it. I am very likely out of free time until the end of the month though, but I won't forget this PR!

NeaMitika · 2020-06-14T14:02:32Z

No problem, i guess we have time untill the new release, maybe in the meantime i could modify the backend too... This way the user is getting a software with a nice UX out of the box.

I still think we should give a brand identity to the software... This way it will be more known... but im out of ideas right now... I am doing some brainstorming when im bored... 😅

Oh yea and btw Italian language is 99.99% translated! ♥️

emilengler · 2020-06-14T22:13:46Z

Thanks for the translation!
I will add it to the next release.
Yes we definitely need a better brand but I am not really good at graphics design but I am open to any suggestions :)

ryangurn · 2020-06-17T17:30:12Z

PodcastGenerator/admin/navbar.php

    <a class="navbar-brand" href="index.php">Admin</a>
    <button class="navbar-toggler" type="button" data-toggle="collapse" data-target="#navbarNav" aria-controls="navbarNav" aria-expanded="false" aria-label="<?php echo _('Toggle navigation'); ?>">
        <span class="navbar-toggler-icon"></span>
    </button>
    <div class="collapse navbar-collapse" id="navbarNav">
        <ul class="navbar-nav">
            <li class="nav-item">
-                <a class="nav-link" href="index.php">Home</a>
+                <a class="nav-link" href="../index.php">Home</a>


Why are we changing the path of index.php to the parent directory?

Right...
I have nested the navbar inside a container (looks a bit more clear) making this, the navbar is less wider and i couldnt fit all the previous menu links inside it...
Since the "Admin" and "Home" in the administrator navbar were pointing to the same link i have removed "View Podcast" which pointed to the main homepage, and modified "Home" so it will point to the main page (like the "view podcast" button did previously).

Hope i made it clear :)

Please leave the View Podcast button as it is, the home button is intended to access the homepage of the admin interface (with the news blog)

ryangurn · 2020-06-17T17:37:44Z

PodcastGenerator/themes/default/navbar.php

                <li class="nav-item">
-                    <a class="nav-link" href="categories.php"><?php echo $categories; ?></a>
+                    <a class="nav-link" href="<?php echo $config['indexfile']; ?>">Home</a>


Was it intentional that we are changing the categories page link to the home page?

Was it intentional that we are changing the categories page link to the home page?

i dont remember making this change... it doesnt make sense to me...

Please revert this change

I strugle to find this code, i dont have it in my navbar.php (i guess i did something wrong 👎 ) how can i fix this?

ryangurn · 2020-06-17T17:38:18Z

PodcastGenerator/themes/default/navbar.php

+                if (strtolower($config["categoriesenabled"]) == "yes") {
+                ?>
+                    <li class="nav-item">
+                        <a class="nav-link" href="categories.php"><?php echo $categories; ?></a>


Looks like you relocated the categories link, makes a bit of sense!

ryangurn

The code looks good, there are some questions I left as comments about some decisions that were made, @emilengler will probably be completing the final review in the future, but I wanted to get you some feedback!

@NeaMitika This looks wonderful, im excited to see the dark theme too.

NeaMitika · 2020-06-17T18:09:08Z

The code looks good, there are some questions I left as comments about some decisions that were made, @emilengler will probably be completing the final review in the future, but I wanted to get you some feedback!

@NeaMitika This looks wonderful, im excited to see the dark theme too.

@ryangurn Hell yeah!!!
It's nice to see other people get involved in this (i saw ur pull request).
I am working on improvements to the Administrative page to make it nice as the main page... but it will take a while...
The Dark Theme should be easy to make, but i will work on that after we finish the Light theme 👍

emilengler

So this is my first fundamental review of this PR. I have not tested it so far but it looks good! I will do a more detailed review later once I tested this more in detail.
Please fix the things I adjusted in this fundamental review

emilengler · 2020-06-17T18:33:13Z

PodcastGenerator/admin/navbar.php

@@ -1,12 +1,13 @@
-<nav class="navbar navbar-expand-lg navbar-dark bg-danger">
+<nav class="navbar navbar-expand-lg navbar-dark bg-dark">


I would not use a dark background honestly. I think the admin interface/bar should differ from the default theme

emilengler · 2020-06-17T18:34:28Z

PodcastGenerator/admin/navbar.php

    <a class="navbar-brand" href="index.php">Admin</a>
    <button class="navbar-toggler" type="button" data-toggle="collapse" data-target="#navbarNav" aria-controls="navbarNav" aria-expanded="false" aria-label="<?php echo _('Toggle navigation'); ?>">
        <span class="navbar-toggler-icon"></span>
    </button>
    <div class="collapse navbar-collapse" id="navbarNav">
        <ul class="navbar-nav">
            <li class="nav-item">
-                <a class="nav-link" href="index.php">Home</a>
+                <a class="nav-link" href="../index.php">Home</a>


Please leave the View Podcast button as it is, the home button is intended to access the homepage of the admin interface (with the news blog)

PodcastGenerator/setup/index.php

emilengler · 2020-06-17T18:39:53Z

PodcastGenerator/themes/default/categories.php

-    <meta charset="utf-8">
+    <link rel="stylesheet" href="<?php echo htmlspecialchars($config["theme_path"]); ?>style/bootstrap.css">    
+    <link rel="stylesheet" href="<?php echo htmlspecialchars($config["theme_path"]); ?>style/custom.css">
+    <link href="https://stackpath.bootstrapcdn.com/font-awesome/4.7.0/css/font-awesome.min.css" rel="stylesheet" integrity="sha384-wvfXpqpZZVQGK6TAh5PVlGOfQNHSoD2xbE+QkPxCAFlNEevoEH3Sl0sibVcOQVnN" crossorigin="anonymous">    <meta charset="utf-8">


This line is problematic.
I don't think using CDNs is a good idea especially because we need to provide a privacy policy then.
I would suggest to download that file and put it into PodcastGenerator/themes/default/style/font-awesome.min.css.
And then change the line to this

<link rel="stylesheet" href="<?php echo htmlspecialchars($config["theme_path"]); ?>style/font-awesome.min.css"

Also please add a linebreak before the tag

emilengler · 2020-06-17T18:55:15Z

PodcastGenerator/themes/default/index.php

@@ -4,6 +4,8 @@
 <head>
    <title><?php echo htmlspecialchars($config["podcast_title"]); ?></title>
    <link rel="stylesheet" href="<?php echo htmlspecialchars($config["theme_path"]); ?>style/bootstrap.css">
+    <link rel="stylesheet" href="<?php echo htmlspecialchars($config["theme_path"]); ?>style/custom.css">
+    <link href="https://stackpath.bootstrapcdn.com/font-awesome/4.7.0/css/font-awesome.min.css" rel="stylesheet" integrity="sha384-wvfXpqpZZVQGK6TAh5PVlGOfQNHSoD2xbE+QkPxCAFlNEevoEH3Sl0sibVcOQVnN" crossorigin="anonymous">


Same here as well

PodcastGenerator/themes/default/listepisodes.php

emilengler · 2020-06-17T18:56:38Z

PodcastGenerator/themes/default/navbar.php

                <li class="nav-item">
-                    <a class="nav-link" href="categories.php"><?php echo $categories; ?></a>
+                    <a class="nav-link" href="<?php echo $config['indexfile']; ?>">Home</a>


Please revert this change

emilengler · 2020-06-17T18:58:44Z

PodcastGenerator/themes/default/theme.json

@@ -1,8 +1,8 @@
 {
-    "name": "default",
-    "description": "The default theme, simple and modern",
+    "name": "Light",


Just a note to myself. Once the dark theme is out we should move the theme to from default to a light directory. Will give more details later (because of our best friends - backwards compatibility :) )

PodcastGenerator/themes/default/theme.json

Made the navbar background blue (red was too much, but i agree that it should be different from the main navbar, as an alternative green works well too) Reversed the changes to "Home" button and "View Podcast" button Since i made the navbar nested inside a container class i strugle with the space and in my case the last 2 buttons in the navbar are on 2 rows. I will try to find a solution before mergin this pull

I had to add the fonts folder as requierd by fontawsome modified the categories.php and index.php of the theme as requested by Emil

baby steps, i will be more carefull in the future

Full time co-author of the Light Theme 👍

ryangurn · 2020-06-21T04:18:43Z

PodcastGenerator/admin/navbar.php

@@ -7,7 +7,7 @@
    <div class="collapse navbar-collapse" id="navbarNav">
        <ul class="navbar-nav">
            <li class="nav-item">
-                <a class="nav-link" href="../index.php">Home</a>
+                <a class="nav-link" href="index.php">Home</a>


ryangurn

This is looking better!

ryangurn · 2020-06-21T04:19:17Z

PodcastGenerator/admin/navbar.php

-                <a class="nav-link active p-1" href="logout.php">Logout - <?php echo $_SESSION["username"]; ?></a>
+        <ul class="navbar-nav">
+            <li class="nav-item">
+                <a class="nav-link" href="<?php echo $config['url']; ?>" target="_blank"><?php echo _('View Podcast'); ?></a>


Is this intended to go to the main podcast page?

I believe yes... it is inside the admin area..

ryangurn · 2020-06-21T04:19:36Z

PodcastGenerator/setup/index.php

@@ -28,7 +28,7 @@
                ?>
                    <div class="alert alert-danger" role="alert">
                        WARNING!: You use a development version of Podcast Generator!<br>
-                            Please use a release version rather than this. You can find them <a class="alert-link" href="https://github.com/PodcastGenerator/PodcastGenerator/releases" target="_blank">here</a>
+                        Please use a release version rather than this. You can find them <a class="alert-link" href="https://github.com/PodcastGenerator/PodcastGenerator/releases" target="_blank">here</a>


Thanks for fixing the tab.

ryangurn · 2020-06-21T04:20:32Z

PodcastGenerator/themes/default/theme.json

@@ -3,6 +3,6 @@
    "description": "The Light theme, simple and modern",
    "version": "3.1",
    "pg_versions": ["3.1"],
-    "author": "Emil Engler, with some help from Manoliu Lucian",
+    "author": "Emil Engler, Manoliu Lucian",


emilengler · 2020-06-21T09:07:31Z

Ok I will do the final review with testing later today. Found some time

NeaMitika · 2020-06-21T09:17:42Z

Ok I will do the final review with testing later today. Found some time

Very well, one thing though, do not include the (PodcastGenerator/admin/navbar.php ), let make this PR only about the Default Theme, i know i have made a mess and included all the stuff here, but it was my first PR, i have learned more in the meantime...

I see now that i can make a PR "work in progress" and after i have finished all the stuff put it in "ready for review" in the next PR i will try to use this feature..

BTW: Bootstrap v5 (alpha) was released... when the final version will be out i cant wait to use it (they have documented better the .sass) we could change colors and stuff

emilengler · 2020-06-21T09:22:53Z

Very well, one thing though, do not include the (PodcastGenerator/admin/navbar.php ), let make this PR only about the Default Theme, i know i have made a mess and included all the stuff here, but it was my first PR, i have learned more in the meantime...

No problem, we were all beginners some day :)
Luckily thanks to git it is easy to reset the changes in the admin files

emilengler · 2020-06-21T10:07:20Z

Your first commit (fde940b) collides with a commit I already merged a few weeks ago (f9ea523)

Should I undo the commit and use your new one?

NeaMitika · 2020-06-21T10:11:38Z

Your first commit (fde940b) collides with a commit I already merged a few weeks ago (f9ea523)

Should I undo the commit and use your new one?

Sure, do as you think is better!

emilengler

Thanks, I merged it with some minimal adjustments.
I nearly cried when I saw it. It looks so beatiful now. Thank you a lot.
Also thanks to @ryangurn for your review, I put a dedication to you in the commit message.

This reverts commit f9ea523. It is done in order to merge #282

emilengler · 2020-06-21T15:32:59Z

Merged now, thank you a lot

emilengler · 2020-06-21T15:34:29Z

By the way, if someone wants to help out, he can replace the preview screenshot of the default theme

NeaMitika added 4 commits June 6, 2020 20:16

HTML improvements for the login.php

fde940b

I have made some modifications to HTMl so it can be more user-friendly.

Working on improvements of the default theme

b4a29da

Improvements to the HTML of the list of episodes

Adjusted the nav menu

6956b84

Improvements for the Default theme aka Light theme

3ae535f

NeaMitika marked this pull request as ready for review June 8, 2020 20:17

Installation step improvements to the HTMl

25ed5df

Some modifications to the navbar

2bf381c

ryangurn reviewed Jun 17, 2020

View reviewed changes

emilengler requested changes Jun 17, 2020

View reviewed changes

NeaMitika added 5 commits June 17, 2020 23:32

Removed extra indent on line 31

7391947

Removed CDN of fontawsome

66cdf21

I had to add the fonts folder as requierd by fontawsome modified the categories.php and index.php of the theme as requested by Emil

Removed the whitespace on line 13

65211ff

baby steps, i will be more carefull in the future

Alright, alright, alright!

14c2562

Full time co-author of the Light Theme 👍

ryangurn reviewed Jun 21, 2020

View reviewed changes

emilengler approved these changes Jun 21, 2020

View reviewed changes

emilengler added a commit that referenced this pull request Jun 21, 2020

Revert "admin: HTML improvements for the login.php"

fffc0b7

This reverts commit f9ea523. It is done in order to merge #282

emilengler closed this in 34bfb74 Jun 21, 2020

kalvinmattheus mentioned this pull request Jun 22, 2020

Add search bar & hide unnecessary pagination & show scheduled episodes to admin #290

Closed

2 tasks

		@@ -1,12 +1,13 @@
		<nav class="navbar navbar-expand-lg navbar-dark bg-danger">
		<nav class="navbar navbar-expand-lg navbar-dark bg-dark">

Improvements for the Default theme #282

Improvements for the Default theme #282

Conversation

NeaMitika commented Jun 8, 2020 • edited

NeaMitika commented Jun 8, 2020

emilengler commented Jun 10, 2020

emilengler commented Jun 14, 2020

NeaMitika commented Jun 14, 2020 • edited

emilengler commented Jun 14, 2020

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

ryangurn left a comment

Choose a reason for hiding this comment

NeaMitika commented Jun 17, 2020

emilengler 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

ryangurn 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

emilengler commented Jun 21, 2020

NeaMitika commented Jun 21, 2020

emilengler commented Jun 21, 2020

emilengler commented Jun 21, 2020

NeaMitika commented Jun 21, 2020 • edited

emilengler left a comment

Choose a reason for hiding this comment

emilengler commented Jun 21, 2020

emilengler commented Jun 21, 2020

NeaMitika commented Jun 8, 2020 •

edited

NeaMitika commented Jun 14, 2020 •

edited

NeaMitika commented Jun 21, 2020 •

edited