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

Calendar page #37

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Calendar page #37

wants to merge 10 commits into from

Conversation

S-Norii
Copy link
Contributor

@S-Norii S-Norii commented Sep 11, 2024

Working calendar, only backend is left to do

Copy link

vercel bot commented Sep 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
program-sch-frontend ❌ Failed (Inspect) Sep 11, 2024 3:36pm

Copy link
Collaborator

@mozsarmate mozsarmate left a comment

Choose a reason for hiding this comment

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

szép munka! jól néz ki, meg nagyon ügyes is a custom render

mobilon még kicsit szétcsűszik, lehetne ott kisebb a margó pl, meg a napokból talán csak a kezdő betűk látszódjanak

majd nem fogod tudni még merge-ni, mert vannak conflict-ok, ezeket szerintem úgy a legegyszerűbb feloldani, hogy előbb a master-t mergeled bele ebbe, itt lokálisan feloldod a konfliktokat, felpusholod és utána mergeled ezt vissza a masterbe

nice job!


import { styles } from '@/components/calendarStyles';

// Segédfüggvények

Choose a reason for hiding this comment

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

This comment is unnecessary


// Calendar komponens
const Calendar: React.FC = () => {
const [currentMonth, setCurrentMonth] = useState(7); // Augusztus

Choose a reason for hiding this comment

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

This should be dynamic as well, we don't want to manually update the month and the year every time it changes :D
Consider using something like const currentDate = new date() and please get the month and year from this one

};

window.addEventListener('resize', handleResize);
handleResize(); // Csak egyszer hívjuk meg, amikor a komponens betöltődik

Choose a reason for hiding this comment

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

this comment is a bit misleading, it is called in case of the resize event as well, so please remove it

Copy link

@DannySS123 DannySS123 left a comment

Choose a reason for hiding this comment

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

Nice work! 😄

I left some comments, and as Máté said, please resolve the merge conflicts, it is a bit hard to see what the new code in the PR is.

The mobile view will need some upgrades too, but be careful with styles like this:
width: '91.666667%'
It's hard to work with, try using full values or margins, or you can check out the calculate() function for widths and heights.

I'd avoid using Hungarian comments in the code, also we don't have to add a comment to every single line. Use comments when it helps to understand the code or it has some additional information to the code. Also don't use comments to mark different parts of a file, if it belongs there then usually no comments are needed, but if it doesn't then it should probably be in a different file.

Otherwise, it looks nice, keep up the good work! 🔥

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.

3 participants