Skip to content
This repository has been archived by the owner on Nov 7, 2022. It is now read-only.

Improve styles #14

Merged
merged 4 commits into from
Apr 11, 2018
Merged

Improve styles #14

merged 4 commits into from
Apr 11, 2018

Conversation

yuku
Copy link

@yuku yuku commented Apr 11, 2018

  • Introduce $qiita-slide-mode- prefix to Sass variables. Global variables always face to collision problem. Seriously.
  • All package variables should be declared as !default.
  • Remove unused rules.
  • Enable to customize z-index of .fullscreen. Proper z-index value strongly depends on where this package runs on.

Copy link
Contributor

@potato4d potato4d left a comment

Choose a reason for hiding this comment

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

@yuku-t

The user is supposed to read _core.scss only (it is described in the README)
The scss variable is a setting to enable external injection, but is it necessary to change the variable name on it?

@yuku
Copy link
Author

yuku commented Apr 11, 2018

The user is supposed to read _core.scss only (it is described in the README)

README describes as follows:

image

Ok, let's suppose you are right (in other words README is wrong), then where the following $break-point-xs comes from? Who defines it? To import the _core.scss, users must define $break-point-xs variable isn't it?

@media (max-width: $break-point-xs) {

@potato4d
Copy link
Contributor

@yuku-t

Sorry. It was supposed to be the default value.

How about setting _vars as the default value and specifying in the README that if you want to expand it, ask that you overwrite it?

Copy link
Contributor

@potato4d potato4d left a comment

Choose a reason for hiding this comment

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

Since it is considered that it is good to change the name of course if it is set as the default value, I think that it is good in the direction of the variable name change.

@yuku
Copy link
Author

yuku commented Apr 11, 2018

It was supposed to be the default value.

This is the reason why I set !default on them.

How about setting _vars as the default value and specifying in the README that if you want to expand it, ask that you overwrite it?

I think there is no difference between your opinion and this PR.

Suppose I want to override $qiita-slide-mode-fullscreen-z-index only, I can write:

$qiita-slide-mode-fullscree-z-index: 10;

@import "~@increments/qiita-slide-mode/src/styles/vars";
@import "~@increments/qiita-slide-mode/src/styles/core";

@yuku yuku merged commit 7040cb9 into master Apr 11, 2018
@yuku yuku deleted the improve-styles branch April 11, 2018 03:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants