Skip to content
This repository has been archived by the owner on Apr 20, 2021. It is now read-only.

Sort z layers with a Sass map. #84

Closed
wants to merge 3 commits into from
Closed

Conversation

kikoruiz
Copy link
Member

WiP This fixes #83.

'cookie-banner': 50,
'ad-smartbanner': 10,
'card-article-icon': 1,
'form-autocompleted-suggests': 1
Copy link

Choose a reason for hiding this comment

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

This two may or may not collide? I understand you are just simply refactoring out... but its worth setting form-autocompleted-suggest at least to 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I would do it, but as you say, it wasn't just a refactor, and it should be tested carefully. Firstly I wanted to set the basis in order to have all z-index values sorted, it's much much more than what we have now 😄 .

@@ -0,0 +1,3 @@
@function z($layer) {
Copy link

Choose a reason for hiding this comment

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

btw, why not z-index?

I really dislike the approach of making everything short... we are not saving bytes in the compilation and makes stuff harder to understand :(

Copy link
Member Author

@kikoruiz kikoruiz Aug 31, 2017

Choose a reason for hiding this comment

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

It's not about saving bytes, it's about not writing names too long, the same reason we're following emmet abbreviations for variable names: https://docs.emmet.io/cheat-sheet/. I have just followed this standard but I'm open to change it if the most of @SUI-Components/developers don't feel comfortable with it in this case.

Copy link

Choose a reason for hiding this comment

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

Guess is a discussion for a total different than this PR so go ahead.

I just that I personally don't understand this decision at all 😢

Copy link
Contributor

@davidbarna davidbarna Aug 31, 2017

Choose a reason for hiding this comment

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

emmet is a global convention well know for naming of common aspects.
IMO we should not create new shorthands for specific stuff.
We should stick to the ones of the convention as exceptions.
Not really related to this task, of course.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the emmet approach. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally would prefer a descriptive name, at least for functions

@@ -156,6 +158,15 @@ $bdrs-small: ceil($bdrs-base / 2) !default;
$bxsh-spread: 3px !default;
$bxsh-base: 0 1px $bxsh-spread rgba($c-black, .15) !default;

// Layers (z-index) sorting.
$z-layers: (
'modal-basic': 100
Copy link
Contributor

@midudev midudev Aug 31, 2017

Choose a reason for hiding this comment

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

I would like to have these sorted alphabetically as it seems that's not scalable at all having those random. :D

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it really makes more sense to have it by z-index value, it gives us a "graphic" status of all the layers sorting, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@midudev midudev Aug 31, 2017

Choose a reason for hiding this comment

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

But then, maybe it would be nice to have the tabs prepared to be in the same line for all the values!

'modal-basic':                      100, 
'cookie-banner':                     50,
'range-datepicker-icon':              2,

To have it visually more sorted! :D

@@ -7,3 +7,4 @@
@import './utils/text';
@import './utils/url';
@import './utils/colors';
//@import './utils/z-index';
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment here!

@midudev midudev closed this Nov 12, 2019
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.

Many z-index values defined throughout the theme setting without a clear sorting.
6 participants