Add some css decorations #38

andrejsharapov · 2023-01-30T21:11:02Z

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

replace svg-background in Hero component to css pattern;
align to edge and add some dynamic for LibraryCard's;
change waves in ``HomeSection`;
other minor changes.

Before submitting the PR, please make sure you do the following

Read the Contributing Guidelines.
Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #yyy).

mririgoyen · 2023-01-30T22:26:17Z

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"! 🤣

mririgoyen

Generally, I think the style changes are good.

I like the new grid, the removal of the SVG assets, and the extra wave.
I like dropping the containing box around the library cards.

What I would like to see changed:

Remove the animation on the hover of "Scroll to Learn More."
Remove the all hover states on the library cards. I think they fit the style of the entire website uncontained.

Bugs:

Please check the site in responsive sizes. There is currently a horizontal scroll. It looks likely due to the width of the buttons in the library cards.

Another Notes:

As mentioned in the code comments, it doesn't look like the changes went through our stylelinter. Please run npm run lint to find errors in our rules. I'll be working on adding that to the pipeline next so we can enforce consistent code.

mririgoyen · 2023-01-31T01:59:59Z

components/Header/Header.module.scss

  padding: 1.5rem;
+  background-color: hsl(var(--secondary-color));


It looks like you may not have had the stylelint plugin working when making these changes. A lot of rules are arbitrarily reordered when they should be alphabetical. Please make sure everything is passing npm run lint before committing. I'll get a pipeline job in place to address this shortly.

mririgoyen · 2023-01-31T02:03:11Z

components/Header/Header.module.scss


  .wordmark {
    display: none;
  }

  .monogram {
-    width: 35px;
+    width: 2.1875rem;


Is there a reason you converted many px values to rem? There's very specific places where px are preferred, namely around graphic sizing which, in this example, is the case. I'd prefer to see px to remain in px unless it directly affects font-size.

mririgoyen · 2023-01-31T02:04:04Z

components/Header/Header.module.scss


    & :not(:last-child) {
-      border-bottom: 1px solid hsl(var(--light-grey));
+      border-bottom: thin solid hsl(var(--light-grey));


No, please use exact values.

mririgoyen · 2023-01-31T02:05:48Z

components/Hero/Hero.module.scss

-@media only screen and (max-width: $mobile-width) {
-  section:not(.full) {
-    background-position: 0 2rem;
+@media ($mobile-width >= width) {


Please leave the media queries as they were written.

mririgoyen · 2023-01-31T02:06:11Z

components/Hero/Hero.tsx

+
+  '&:hover': {
+    letterSpacing: 1.1,
+  },


Please revert this change.

mririgoyen · 2023-01-31T02:08:14Z

components/HomeSection/HomeSection.module.scss

+    font: {
+      size: 2.5rem;
+      weight: 600;
+    }


While I appreciate the ability to nest rules in this way, it makes using automatic alphabetizing more difficult and I would prefer we do not do this.

mririgoyen · 2023-01-31T02:09:33Z

components/HomeSection/HomeSection.module.scss

    z-index: 1;
+    width: 100%;
+    height: 6.25rem;
+    background-color: hsl(var(--primary-color) / 100%);


The 100% isn't necessary here.

mririgoyen · 2023-01-31T02:10:26Z

components/HomeSection/HomeSection.module.scss

  }
 }

-@media only screen and (max-width: $mobile-width) {
+@media (46rem >=width) {


No magic numbers please. Where is 46rem coming from?

mririgoyen · 2023-01-31T02:11:36Z

components/LibraryCard/LibraryCard.module.scss

@@ -32,13 +43,14 @@
  .image {
    position: relative;
    width: 100%;
+    transition: all 0.25s linear;


Please remove this transition.

andrejsharapov · 2023-01-31T06:47:43Z

Good. Today I will make all the corrections.

A few more wishes:

It would be nice to make a template for PR.
Add a CONTRIBUTING file or create a Wiki so other members can follow the instructions and not make the same mistakes I did.

mririgoyen · 2023-01-31T15:29:22Z

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/.

andrejsharapov · 2023-01-31T19:47:15Z

Our contributing information is linked in the README: pictogrammers.com/docs/contribute/website.

I meant something like Code style mentioning stylelint.

mririgoyen · 2023-01-31T19:56:43Z

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. 😄

mririgoyen

There's still some code changes that need to happen here. Once they're done, I will pull down the branch to do some manual testing before giving it the 👍🏼. There are also merge conflicts that need to be resolved.

components/Header/Header.module.scss

components/Hero/Hero.module.scss

components/HomeSection/HomeSection.module.scss

mririgoyen · 2023-02-01T01:29:44Z

components/LibraryCard/LibraryCard.tsx

  backgroundColor: 'white',
  borderBottom: 'none',
+  borderRadius: 24,


The pill shape of the button does not match any other button's style on the site, so let's remove this.

Suggested change

borderRadius: 24,

The site navigation and the "Scroll to Learn More" button have a more circular shape.

Only on hover. The header follows that pattern to match the search box. The hero does it to match the header because of proximity. Neither area really "buttons." All elements that look like buttons should follow the Material guidelines as per the MUI <Button/> component.

styles/pages/index.module.scss

mririgoyen · 2023-02-01T01:31:30Z

styles/pages/index.module.scss

-  grid-template-columns: repeat(auto-fit, minmax(320px, 1fr));
-  justify-items: center;
+  grid-gap: 3rem;
+  grid-template-columns: repeat(auto-fit, minmax(var(--col-size), 1fr));


Let's not use CSS variables here and be explicit here and in the media query. I get what you're doing here, but its just not a pattern we've done in this repo.

mririgoyen · 2023-02-01T01:31:55Z

components/LibraryCard/LibraryCard.module.scss

 }

 .content {
  .image {
    position: relative;
+    transition: all .2s linear;


Still need to remove this transition.

Transition smoothes the behavior of the icon when hovering over the card.

What I'm asking is to remove all transitions from those cards. Keep the style as you have proposed before hover, but make the buttons the only clickable areas. 😄

Co-authored-by: Michael Irigoyen <[email protected]>

andrejsharapov · 2023-02-01T21:10:25Z

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/or em 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.

mririgoyen · 2023-02-02T01:59:38Z

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?

andrejsharapov · 2023-02-02T13:43:04Z

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 ✌️

andrejsharapov added 7 commits January 28, 2023 20:33

chore(dev): replace svg-background to css pattern

a43fea1

Merge branch 'main' into change-design-elements

6f7a463

chore(style): update background

3b636d6

chore(style): change header

53b8b9b

chore(style): align to edge and add some dynamic

33b46ee

chore(style): add some decorations

a926e7a

chore(fix): replace 1px

df7eafe

mririgoyen requested changes Jan 31, 2023

View reviewed changes

andrejsharapov changed the title ~~chore(style): add some css decorations~~ Add some css decorations Jan 31, 2023

Fix code with stylelint

98c61a0

Rollback changes

154461c

Rollback changes

4031ea4

mririgoyen requested changes Feb 1, 2023

View reviewed changes

andrejsharapov and others added 10 commits February 1, 2023 23:18

Update components/Hero/Hero.module.scss

2e51363

Co-authored-by: Michael Irigoyen <[email protected]>

Replace rem to px

4c760ff

Co-authored-by: Michael Irigoyen <[email protected]>

Replace rem to px

223f7c1

Co-authored-by: Michael Irigoyen <[email protected]>

Merge branch 'main' into change-design-elements

a85f0d2

Replace rem to px

224e36c

Replace rem to px

8fb9f25

Remove CSS variables

1852dd4

Remove border-radius and fix other styles

236cd2e

Remove transition from images

d834cbf

Remove css transform

660b81f

mririgoyen mentioned this pull request Feb 2, 2023

Change design elements #45

Merged

mririgoyen closed this in #45 Feb 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some css decorations #38

Add some css decorations #38

andrejsharapov commented Jan 30, 2023

mririgoyen commented Jan 30, 2023

mririgoyen left a comment

mririgoyen Jan 31, 2023

mririgoyen Jan 31, 2023

mririgoyen Jan 31, 2023

mririgoyen Jan 31, 2023

mririgoyen Jan 31, 2023

mririgoyen Jan 31, 2023

mririgoyen Jan 31, 2023

mririgoyen Jan 31, 2023

mririgoyen Jan 31, 2023

andrejsharapov commented Jan 31, 2023

mririgoyen commented Jan 31, 2023

andrejsharapov commented Jan 31, 2023

mririgoyen commented Jan 31, 2023

mririgoyen left a comment •

edited

Loading

mririgoyen Feb 1, 2023

andrejsharapov Feb 1, 2023

mririgoyen Feb 1, 2023

mririgoyen Feb 1, 2023

mririgoyen Feb 1, 2023

andrejsharapov Feb 1, 2023

mririgoyen Feb 1, 2023

andrejsharapov commented Feb 1, 2023

mririgoyen commented Feb 2, 2023

andrejsharapov commented Feb 2, 2023 •

edited

Loading

		padding: 1.5rem;
		background-color: hsl(var(--secondary-color));

Add some css decorations #38

Add some css decorations #38

Conversation

andrejsharapov commented Jan 30, 2023

What is the purpose of this pull request?

Description

Checklist

Before submitting the PR, please make sure you do the following

mririgoyen commented Jan 30, 2023

mririgoyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrejsharapov commented Jan 31, 2023

mririgoyen commented Jan 31, 2023

andrejsharapov commented Jan 31, 2023

mririgoyen commented Jan 31, 2023

mririgoyen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrejsharapov commented Feb 1, 2023

mririgoyen commented Feb 2, 2023

andrejsharapov commented Feb 2, 2023 • edited Loading

mririgoyen left a comment •

edited

Loading

andrejsharapov commented Feb 2, 2023 •

edited

Loading