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

10-final_Allocene #424

Merged
merged 16 commits into from
Dec 29, 2023
Merged

10-final_Allocene #424

merged 16 commits into from
Dec 29, 2023

Conversation

Allocene
Copy link
Collaborator

No description provided.

@vadym-zinchenko-moc vadym-zinchenko-moc added the Ready to final review The pr is ready for review of random mentor label Dec 25, 2023
@vladyslav-yermolin-moc
Copy link
Collaborator

Hey, @Allocene, please add a link to your deloyed project, as other students do #434 (comment)

@Allocene
Copy link
Collaborator Author

Allocene commented Dec 25, 2023

I deployed my project through a third-party resource. Here is a link to the project - Artur Kobyliatsky (click to open)

Copy link
Collaborator

@vladyslav-yermolin-moc vladyslav-yermolin-moc left a comment

Choose a reason for hiding this comment

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

Please change files only in the docs folder.
Try to remove changes of files in folder homeworks/artur... from this pull request

docs/Allocene/games.html Show resolved Hide resolved
docs/Allocene/img/Call_of_Duty-_Modern_Warfare_II.jpeg Outdated Show resolved Hide resolved
docs/Allocene/img/jpt-hero-desktop.jpg Outdated Show resolved Hide resolved
docs/Allocene/login-success.html Outdated Show resolved Hide resolved
docs/Allocene/script/games.js Outdated Show resolved Hide resolved
docs/Allocene/script/games.js Outdated Show resolved Hide resolved
docs/Allocene/script/games.js Show resolved Hide resolved
docs/Allocene/script/games.js Show resolved Hide resolved
docs/Allocene/script/games.js Outdated Show resolved Hide resolved
docs/Allocene/style/main-login.css Show resolved Hide resolved
docs/Allocene/style/main-about.css Outdated Show resolved Hide resolved
docs/Allocene/style/main-about.css Outdated Show resolved Hide resolved
docs/Allocene/style/main-calculator.css Outdated Show resolved Hide resolved
docs/Allocene/style/main-calculator.css Outdated Show resolved Hide resolved
docs/Allocene/style/main-games.css Outdated Show resolved Hide resolved
docs/Allocene/style/main-games.css Outdated Show resolved Hide resolved
docs/Allocene/style/main-games.css Outdated Show resolved Hide resolved
docs/Allocene/style/main-games.css Outdated Show resolved Hide resolved
docs/Allocene/style/header.css Outdated Show resolved Hide resolved
Copy link
Collaborator

@vladyslav-yermolin-moc vladyslav-yermolin-moc left a comment

Choose a reason for hiding this comment

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

I have a strange feeling: seems like you want to compete with me or other mentors and try to avoid fixing your code on our comments.
This is not a competition. We like to teach you something useful, based on our experience.
If you don't like our comments, I can merge your request "as is" and save my and your time, not a problem🤷‍♂️
P.S.: please do not resolve my comments, anyway I should open them and check your fixes. Better add fixed comment

docs/Allocene/style/main-login.css Outdated Show resolved Hide resolved
docs/Allocene/style/main-login.css Outdated Show resolved Hide resolved
docs/Allocene/style/main-loginSuccess.css Outdated Show resolved Hide resolved
docs/Allocene/script/burger.js Outdated Show resolved Hide resolved
docs/Allocene/script/burger.js Outdated Show resolved Hide resolved
docs/Allocene/script/burger.js Outdated Show resolved Hide resolved
docs/Allocene/script/games.js Outdated Show resolved Hide resolved
docs/Allocene/script/games.js Show resolved Hide resolved
docs/Allocene/script/games.js Show resolved Hide resolved
@Allocene
Copy link
Collaborator Author

I'm sorry that it seems that way, because in reality it's not like that, it's simple and I never understand why and I'm waiting for some logical explanations, but I gladly make all the corrections, because I understand that this is experience, and this is exactly the small work that you talked about last time

@vladyslav-yermolin-moc
Copy link
Collaborator

Hey, @Allocene, thanks for your fixes. I don't clearly understand, what you mean by

it's simple and I never understand why and I'm waiting for some logical explanations,

  • CSS classes are a mechanism to make unique identifiers for your HTML element and add some styles specifically to it without fear, that your styles will be applied to some other HTML element, as it could be when you add styles by tag. The only exception here is some common styles for all tags in your project (e.g. all text has #000 color by default)
  • data attributes are designed for interaction with HTML elements from JS. You will never have a headache about changing CSS-class and broken js-logic as a result because in js you get this HTML element by CSS class.

These two rules are best practices in HTML+CSS+JS(pure) projects

Is this explanation what you were waiting for?

Anyway, would be good for such cases to ask a question in comments on GitHub or in tg, rather than silently ignore or resolve the mentor's comment.

@vladyslav-yermolin-moc vladyslav-yermolin-moc merged commit 5559512 into main Dec 29, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Final reviewer assigned Ready to final review The pr is ready for review of random mentor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants