Skip to content

Use scss nesting & variables#393

Merged
gnikonorov merged 2 commits intopytest-dev:masterfrom
jkowalleck:use_scss_features
Dec 11, 2020
Merged

Use scss nesting & variables#393
gnikonorov merged 2 commits intopytest-dev:masterfrom
jkowalleck:use_scss_features

Conversation

@jkowalleck
Copy link
Copy Markdown
Contributor

@jkowalleck jkowalleck commented Dec 1, 2020

  • used scss variables for reoccur values
  • used scss math to calculate values that depend on variables.

also: scss allowes nesting of css selectors, which was applied here

the CSS was compiled based on changed SCSS.

changes in this PR are related to #383

@jkowalleck
Copy link
Copy Markdown
Contributor Author

Hello @gnikonorov,

you assigned me this ticket.
Is there some action i should take?

@gnikonorov
Copy link
Copy Markdown
Member

Hello @gnikonorov,

you assigned me this ticket.

Is there some action i should take?

Hi @jkowalleck. No action required on your end at all. I assigned you since you're working on it, that's all

@jkowalleck
Copy link
Copy Markdown
Contributor Author

i see, @gnikonorov .
work on this PR is done. ready for review.

@ssbarnea ssbarnea added the feature This issue/PR relates to a feature request. label Dec 3, 2020
@jkowalleck
Copy link
Copy Markdown
Contributor Author

@gnikonorov @ssbarnea @BeyondEvil
this one is ready for review

Copy link
Copy Markdown
Member

@gnikonorov gnikonorov left a comment

Choose a reason for hiding this comment

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

Sorry for falling behind on this on @jkowalleck and thank you for raising! I left a few comments.

@jkowalleck
Copy link
Copy Markdown
Contributor Author

no worries @gnikonorov
changed the scss as requested baed on remarks of #393 (review)
recompiled the scss to css - nothing changed on the outcome.

@jkowalleck jkowalleck requested a review from gnikonorov December 9, 2020 12:44
@jkowalleck
Copy link
Copy Markdown
Contributor Author

jkowalleck commented Dec 9, 2020

@gnikonorov something seams to fail with this build.
https://readthedocs.org/projects/pytest-html/builds/12522208/

i don't see why this is happening with my changes. any advice?
i guess its connected to #401 and the work that is currently done to it.

@gnikonorov
Copy link
Copy Markdown
Member

@gnikonorov something seams to fail with this build.
https://readthedocs.org/projects/pytest-html/builds/12522208/

i don't see why this is happening with my changes. any advice?
i guess its connected to #401 and the work that is currently done to it.

It is @jkowalleck. The issue is gone now since the PR is merged into master

@gnikonorov gnikonorov added code quality This PR has to do with improving code readability/quality ( refactoring, etc. ) and removed feature This issue/PR relates to a feature request. labels Dec 10, 2020
gnikonorov
gnikonorov previously approved these changes Dec 10, 2020
Copy link
Copy Markdown
Member

@gnikonorov gnikonorov left a comment

Choose a reason for hiding this comment

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

Thanks for this @jkowalleck! I had one nitpick, but otherwise LGTM. Feel free to ignore my comment if I misunderstood.

@BeyondEvil or @ssbarnea could one of you take a look for a second review/merge?

.log:only-child {
height: inherit
}
$extra-spacing: $spacing;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just curious, why do we need to define $extra-spacing? Can't we just use $spacing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sure can use $spacing and remove $extra-spacing.
used $extra-spacing i case the original author had other plans for the spacing in different sections.
now, from a readers perspective i would say it is easier to stay with $spacing.

any opinions from others?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove it please if it's not used @jkowalleck 🙏

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done that.

* beatify scss
* applied variables for fonts, borders, spacing
* variables fr exra sizing
* add variable for triagle
* applied scss nesting
Copy link
Copy Markdown
Contributor

@BeyondEvil BeyondEvil left a comment

Choose a reason for hiding this comment

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

Send it! 🚀

Great work everyone! 💪

Copy link
Copy Markdown
Member

@gnikonorov gnikonorov left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @jkowalleck!

@gnikonorov gnikonorov merged commit e00532d into pytest-dev:master Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code quality This PR has to do with improving code readability/quality ( refactoring, etc. )

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants