Closed andrejsharapov closed 1 year ago
I'll take a look at this tonight and provide my feedback.
But may I please ask that you do not implement a git commit message style that we do not intend to follow. It'll make the history look out-of-place if some message follow some sort of style and others do not. As I stated before, we're not planning to adhere to any kind of message style for this project as the overhead does not provide us with any benefits.
P.S. I hope you don't think contributing to us is a "chore"! 🤣
Good. Today I will make all the corrections.
A few more wishes:
- It would be nice to make a template for PR.
I agree, I will be creating these soon. Same for issues.
- Add a CONTRIBUTING file or create a Wiki so other members can follow the instructions and not make the same mistakes I did.
Our contributing information is linked in the README: https://pictogrammers.com/docs/contribute/website/.
Our contributing information is linked in the README: pictogrammers.com/docs/contribute/website.
I meant something like Code style
mentioning stylelint.
Our contributing information is linked in the README: pictogrammers.com/docs/contribute/website.
I meant something like
Code style
mentioning stylelint.
Yep, the documentation needs to be expanded on. There's already an issue open for it. 😄
Looks like it's done)
Note Several interesting observations.
- This turned out to be a little more difficult than I thought for several reasons: I stopped using pixels in my work a long time ago and work with
rem
and/orem
more often. Plus, you're using px and rem on the same "system":// variables.scss // px $tablet-width: 910px; $mobile-width: 736px; // but rem $width-max: 96rem;
- And yet, the choice of such breakpoints is not entirely clear to me, they are very different from the generally accepted ones (768, 992, 1024, 1200, etc.).
- In some places it is preferable to use variables than to duplicate code with different values.
- Many places use questionable pixels like 5/30/33/175 and others. It would be nice to translate them to 4/8/32...
In general, I liked the work. I'll suggest more improvements over time if you don't mind.
Definitely, we love suggestions, feedbacks, and PRs.
My apologies about some of the inconsistencies in terms of px
/rem
. The site was put together by myself over the past 2 months. When there's a lot to do, sometimes details slip through the cracks. There's still a lot to do in terms of features and in terms of Developer Experience.
Let's see if I can address/justify any of your concerns and see if we can find places for improvement.
My philosophy on pixels vs rems is like comparing inches to centimeters. For general "good enough" cases, rems typically do fine. I tend to use rems for padding, margin, gaps, etc., anything that doesn't need to be too precise. Rems or vh/vw always get used for font-sizes for obvious reasons. When you need that precision, pixels are much easier to work with. (e.g. 1px
vs 0.0625rem
) It's less to write, it's easier, it's less bytes in the file. All that being said, there are definitely some inconsistencies that could be cleaned up following that mantra.
There is no hard-and-fast rule on breakpoints. There's lots of information on approximate screen resolution ranges for different devices, but when it comes down to it, breakpoints need to make sense to the website. In our case, the breakpoints are close to the top of the range for each device, but are adjusted slightly to better handle the content we're offering without having to make a ton of little one-off rules.
I agree here and think we can improve on this, but we should do it all at once and then remain consistent.
I'm not sure I understand your concern exactly, why are 5/30/33/175 questionable?
I'm not sure I understand your concern exactly, why are 5/30/33/175 questionable?
Try to avoid odd numbers. You have probably noticed that many modern grids, fonts, and others use even values that are multiples of 4 (8, 12, 16, 24). The Material also uses these values. MUI used on this site is also built on this system. This will also be useful for the future margin/padding system.
This applies to point 1 as well. px
is best used to set the size of decorative elements, e.g. border-width
, but and here I'd better apply the thin, medium or thick
, for everything else (margins, padding, max-/width, media query, font-size etc - rem
.
Indents of 12, 16, 24, 32, 48, 56px and above are easier to use and easy to manipulate:
11px = 0.6875rem;
// vs
12px = 0.75rem;
33px = 2.0625rem;
// vs
32px = 2rem;
or when creating variables/mixins:
$base_px = 0.25rem;
$font_size = calc(#{$base_px} * 4);
In the future, it will become easier to translate and navigate in em/rem
, without 0.0000625
:v:
What is the purpose of this pull request?
Description
I've added some dynamism to the main page and made some improvements to the code.
Checklist
LibraryCard's
;`HomeSection
;Before submitting the PR, please make sure you do the following
fixes #yyy
).