-
Notifications
You must be signed in to change notification settings - Fork 1
Deurdu local #11
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
base: deurdu
Are you sure you want to change the base?
Deurdu local #11
Conversation
|
This branch still contains commits which are already present in my To move this forward, I would try to pick all the changes from be6a6dd into a new branch and open a PR, we can probably merge that branch then. But first please address rest of the review comments. |
fsheikh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in merge-able form. Also please see questions/comments
| // credentials: true // Allow cookies to be sent with requests | ||
| // })); | ||
|
|
||
| // app.use(cookieParser()); | ||
|
|
||
| // const db = mysql.createConnection({ | ||
| // host: 'localhost', | ||
| // user: 'root', | ||
| // database: 'deurdu', | ||
| // password: '' | ||
| // }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is authentication happening if you commented out this part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I was moving the project with JWT Tokens for auth, but after discussing with you, I moved to the auth0 user management system, through which we can handle the users and their authentication.
Therefore, there is not use of this authController.js file, that's why I had commented it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see below comment
|
|
||
|
|
||
|
|
||
| export default router; No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you return a router object if this it is no longer defined in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I missed out to comment it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is currently not needed, please delete this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, the file is deleted.
| const router = express.Router(); | ||
|
|
||
| // SQLite3 database connection | ||
| const db = new sqlite3.Database('deurdu.db', (err) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we define string constants like name of database and the filepaths in a separate file and then use here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is possible.
Let me share you a sample code, for the clear picture.
// config.js
const path = require('path');
module.exports = {
DATABASE_NAME: 'deurdu.db',
UPLOADS_PATH: path.join(__dirname, 'uploads'),
LOGS_PATH: path.join(__dirname, 'logs')
};And then:
const express = require('express');
const sqlite3 = require('sqlite3');
const { DATABASE_NAME } = require('./config');
const router = express.Router();
// SQLite3 database connection
const db = new sqlite3.Database(DATABASE_NAME, (err) => {
if (err) {
console.error('Error connecting to database:', err.message);
} else {
console.log('Connected to the database.');
}
});
module.exports = router;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will be incorporated in reviewed work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, Got it.
I have made a Config.js file in server, where I have initialized the DB_NAME as the string constant.
Also I will add the comments in it for the assistance of you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MuhammadTaha01 where is this new configuration file uploaded, did you pull the branch taha_deurdu_local from #12 before introducing this file? I don´t see any changes added to that pull-request, last change being 5ba0ec6
May be you forgot to push your changes to the correct branch?
| // Configure multer for file uploads | ||
| const storage = multer.diskStorage({ | ||
| destination: (req, file, cb) => { | ||
| cb(null, './pictures'); // Folder to save uploaded images |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the images will be saved in the directory <project-home>/server/Controllers/pictures ? or <project-home>/pictures. Also as mentioned above this path should be defined as a separate constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Paths and some other stuff could be defined in the separate file as the constant.
But, right now project isn't taking pictures in the database, as I told you earlier.
Currently, it is only taking the information of blogs like title, author name, content.
The image space isn't functional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
important is to to define a constant file path relative to server directory where all multimedia data received from Multer is saved. Backend then stores data in the database which maps these pictures to a particular blog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, got it.
I have defined the Image Path as the string constant into the Config.js file.
The uploaded images will go to the derudu/server/Controllers/pictures.
And later on, will work on the functionality to upload the images too.
| }, | ||
| }); | ||
|
|
||
| const upload = multer({ storage }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is this upload object after this definition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I stated in my above response, that uploading image functionality isn't working right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a TBD (To-Be-Done) comment above this line to indicate this still needs some more work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a TBD comment on it, as it needs further implementation
| {/* <Link to={`${getLanguagePath()}/login`}> | ||
| <button className='px-2 py-0.5 m-1 rounded-lg text-white'>{login}</button> | ||
| </Link> | ||
| <Link to={`${getLanguagePath()}/register`}> | ||
| <button className='px-2 py-0.5 m-1 rounded-lg text-white'>{register}</button> | ||
| </Link> */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see commented out code in many places, not sure if this is debugging leftover or not needed any longer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the commented code is for no needed longer.
But, in-case in in future, we think of migrating our webapp from auth0 to JWT Tokens.
It would be beneficial.
| <div className={`flex bg-gray-700 ${i18.language === 'ur' ? 'gap-[775px]' : 'gap-[880px]'} | ||
| ${i18.language === 'de' ? 'gap-[700px]' : 'gap-[880px]'}`}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above how can the developer know how many pixels are needed here, comparing 775px to 700px
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The developer could evaluate that thing as per device, like for large screens (laptops etc...) it is good but for other screens it could be customize.
| gmail: '', | ||
| password: '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
login is only possible with gmail add or any other email address also possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now it is not in use the Login.jsx and Signup.jsx page isn't in use.
As we are using auth0 as user management system.
| axios.post('http://localhost:3030/auth/login', values) | ||
| .then(res => | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the first time login path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not in use as we are using auth0 as the user management system.
| { | ||
| alert('Error while registering the user.'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like the case when user was not authenticated so the error log is misleading. Also where is the handling for registering a first time user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error is for that if the user email and password, isn't find in database so this error will be pop-out.
But, for now it is not important as we are using auth0 as user management system.
| To regenerate the `package.json` file with default values, run the following command in the **server** directory: | ||
|
|
||
| ```bash | ||
| npm init -y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not enough as it just generates an empty package.json file with the following contents
npm init -y
Wrote to /home/q550356/sample_code/server/package.json:
{
"name": "server",
"version": "1.0.0",
"description": "If you've accidentally deleted the `package.json` or `package-lock.json` files in the **server** directory, you can follow these steps to regenerate them.",
"main": "index.js",
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1"
},
"keywords": [],
"author": "",
"license": "ISC"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried using the previous package file from https://github.com/fsheikh/sample_code/blob/deurdu/deurdu/server/package.json but then had to npm install express and npm install sqlite3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok, by the way I did that thing: npm init -y and it generated me that package.json file.
I don't know why, but we will discuss it on call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed, nothing to do here, #12 already has put back this configuration file. Just please use that branch as the base, meaning before making more changes, pull that branch locally taha_deurdu_local
| This directory contains all data and source code for deurdu blogging | ||
| website. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why you deleted the top level deurdu directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't. I was also thinking that when I was just pushing my code to deurdu_branch_local, why the deurdu folder auto get's deleted.
| ### 3️⃣ **Generate Missing Configuration Files** | ||
|
|
||
| As some configuration files were deleted, you'll need to regenerate or create them manually. Here are the necessary steps for each file: | ||
| ## **For TailwindCSS:** | ||
| Regenerate the ```tailwind.config.js``` file: | ||
| ```bash | ||
| npx tailwindcss init | ||
| ``` | ||
| This will generate the ```tailwind.config.js``` file. | ||
|
|
||
| ## **For Vite:** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like each configuration needs a separate step, in this case it would be better to put these configuration in back in code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, it would.
But we can discuss it in deep on call.
| Once everything is set up, you can run the development server with one of the following commands: | ||
| - Using npm: | ||
| ```bash | ||
| npm run dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined means that app was unable to extract the selected lang from params and thus undefined comes there.
By the way the lang is set to be eng by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed it is a bug and needs further investigation.
fsheikh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please address the review comments as discussed on the phone
| Once everything is set up, you can run the development server with one of the following commands: | ||
| - Using npm: | ||
| ```bash | ||
| npm run dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed it is a bug and needs further investigation.
| // Make the database connection available to routes | ||
| app.use((req, res, next) => { | ||
| req.db = db; // Attach the database connection to the request object | ||
| next(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put documentation link in code itself: https://expressjs.com/en/guide/writing-middleware.html
|
|
||
| // Make the database connection available to routes | ||
| app.use((req, res, next) => { | ||
| req.db = db; // Attach the database connection to the request object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we need only one global initialization of sqlite3 database, so the one in https://github.com/fsheikh/sample_code/pull/11/files#diff-9ac2ac1d104a8ac8f861c556d1082ec1051171365a02a5e5355d49265b437177R8 should be removed.
| import express from 'express'; | ||
| import cors from 'cors'; | ||
| import sqlite3 from 'sqlite3'; // Import sqlite3 | ||
| import blogControllerRoutes from './Controllers/blogControllers.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed offline blogControllerRoutes should be renamed to blogRoutes or something similar/simpler. And a comment mentioning that objects are being instantiated here at the top of the file.
| To regenerate the `package.json` file with default values, run the following command in the **server** directory: | ||
|
|
||
| ```bash | ||
| npm init -y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed, nothing to do here, #12 already has put back this configuration file. Just please use that branch as the base, meaning before making more changes, pull that branch locally taha_deurdu_local
|
|
||
| const Cards = ({img,title,description}) => { | ||
| return ( | ||
| <div className='flex flex-col w-[250px] m-6 border-2 border-black text-center p-2 rounded-lg'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add Tailwind documentation link defining styling attributes.
| const {ul1,ul2,ul3,ul4} = t('ulTranslation') | ||
|
|
||
| return( | ||
| <div className="bg-green-200 py-5 flex space-x-[1050px]"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to rerun the full client again, code changes are applied live. Further action pending on testing
| <li className="hover:underline transition cursor-pointer">{ul2}</li> | ||
| <li className="hover:underline transition cursor-pointer">{ul3}</li> | ||
| <li className="hover:underline transition cursor-pointer">{ul4}</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uli2 e.g. is a link still on homepage like AboutUs. Please populate the links with each ul value. Content can be empty but at least links should be established
| return ( | ||
| <div className="slide-container"> | ||
| <Fade> | ||
| {fadeImages.map((fadeImage, index) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please when using built in javascipt functions always add a reference to the API documentation.
|
|
||
| {/* This is the 1st navbar */} | ||
| <div className= "bg-green-700}"> | ||
| <div className={`flex bg-green-700 gap-[1150px] ${i18.language === 'ur' ? 'gap-[1130px]' : 'gap-[1150px'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please fix the syntax error, it also shows that testing is missing


Auto-Generated files were removed and all the
commandsto generate them were being written down(Readme.md)as per their specific files.