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

Style: enhance the style of some platform pages #61

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mostafanasser2000
Copy link
Contributor

Style: enhance the style of some platform pages

Title: CSS enhancements to improve the platform UI.

Description

In this pull request, I try to use ChatGPT to implement several CSS enhancements to improve the platform's UI.

Changes Made

  • enhance the page layout and make pages responsive
  • create a better login page with social icons
  • change how courses are displayed on the website
  • replace links to the github repo and official site of data talks with icons
  • improve homework form

Visual Changes

Screencast.from.2024-07-25.22-26-24.mp4

@alexeygrigorev
Copy link
Member

looks great! can you please ask chatgpt to merge it with the existing styles? and if you can use the existing style classes, it'd be great, so it's easier to track what was changed

@mostafanasser2000
Copy link
Contributor Author

I don't think there will be a problem merging it as styles are added inside the <style> tag in the <head> not inside the CSS file which will not cause using collectstatic command, and the other style classes are bootstrap classes using bootstrap4.5.2 which are used before, so there will no conflict between classes


<style>
body {
Copy link
Member

Choose a reason for hiding this comment

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

can you please move it to the courses.css? and please make sure there are no conflits with the existing styles

Copy link
Member

Choose a reason for hiding this comment

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

or even better to login.css

@@ -1,43 +1,119 @@
{% extends 'base.html' %}
{% load tz %}

{% block styles %}
<style>
.course-header {
Copy link
Member

Choose a reason for hiding this comment

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

same here - please don't keep css in html

@@ -2,13 +2,80 @@

{% load static %}

{% block styles %}
<style>
.homework-container {
Copy link
Member

Choose a reason for hiding this comment

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

and here and everywhere else where we have css in html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok i will move all CSS to external CSS files

<li><a href="{% url 'course' course.slug %}">{{ course.title }}</a></li>
<div class="col mb-4">
<div class="card h-100 shadow hover-effect">
<img src="https://via.placeholder.com/400x250/1E448B/FFF?text={{ course.title|urlencode }}" class="card-img-top" alt="{{ course.title }}" decoding="async">
Copy link
Member

Choose a reason for hiding this comment

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

What's this website? I'm wondering if it's a good idea to rely on something external

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want the course card to have images like those images images so I came up with 3 approaches

  1. add ImageField to the course model and use it on the course card but this approach needs a lot of work

    • migrations and expected problems that may happen because of courses that exist and don't have an image
    • a lot of configurations are needed for Django so that Django can serve images correctly on deployment
  2. create static/imgs folder and add those images by hand and on templates and this requires

    • we should name images using the course name or course slug so that we can add them easily on templates
    • every future course we need to add an image for it on the static/imgs folder
  3. the third approach and the easiest one use third-party websites that create placeholder images that can be added to a course card

Copy link
Member

Choose a reason for hiding this comment

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

Let's do this in a separate PR please. I'd do it via uploading the image to S3 but it's a bigger change for now

@mostafanasser2000
Copy link
Contributor Author

here is what the last commit introduce

  • move all CSS from template to CSS files
  • add a default image for the course card so there is no need to use a website that creates placeholder images anymore

the final preview of the website

preview.mp4

@alexeygrigorev
Copy link
Member

I definitely like it but not everything, and because the change is too large it's too difficult for me to accept it all at once. Is there a way to gradually change it?

@alexeygrigorev
Copy link
Member

Let's maybe start with a PR for the login page?

@mostafanasser2000
Copy link
Contributor Author

ok I will close this pull request retrieve all changes back and create a new one for the login page only, I think focusing on Django related PR's will be helpful at this time

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.

2 participants