Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Advanced styles customization #235

Merged
merged 19 commits into from Oct 6, 2019

Conversation

earshinov
Copy link
Contributor

@earshinov earshinov commented Sep 14, 2019

Hello @madoar ,

It is usually considered a good practice of visual design to restrict colors used in the application UI to a limited subset which might be called a palette or a theme. In some cases, such theme lists not only base colors, but also their shades from light to dark to be used in different parts of the UI.

Version 5.0.0 of angular-archwizard exposes SCSS variables which can be used for customizing base colors of the wizard. However, things like using darken(…, 10%) to derive a hover color from a base color are still hard coded and hard for a library user to override.

This PR extends angular-archwizard's style customization support. Instead of a small number of variables controlling base colors ($wz-color-default, …, $wz-color-editing) it exposes a complex $wz-colors structure that completely defines a color scheme to be used for navbar indicators, including colors for hovered state.

As an auxiliary change, the updated *.scss file also exposes $wz-layouts and $wz-indicator-styles variables. Library users can override these to exclude unused layouts and indicator style and thus optimize CSS output size.

Notes:

  • I have been using this modified version of angular-wizard in order to customize its colors for our project.
  • This PR introduces incompatible changes for those who already customize wizard's appearance using SCSS. For other users, changes will go unnoticed. As a matter of fact, this PR does not introduce any meaningful changes in the CSS file included into angular-archwizard npm package.

Copy link
Owner

@madoar madoar 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 my late review.

Thanks for making some improvements to the color data structures! I like your approach but I have some questions and change requests which I added as comments.

Please bear with some of my more silly questions. I'm not that knowledgeable with SCSS, therefore I've added some questions about parts of the code that I don't (fully) understand.

src/css/archwizard.scss Outdated Show resolved Hide resolved
src/css/archwizard.scss Outdated Show resolved Hide resolved
src/css/archwizard.scss Outdated Show resolved Hide resolved
src/css/archwizard.scss Show resolved Hide resolved
src/css/archwizard.scss Outdated Show resolved Hide resolved
src/css/wizard-navigation-bar.scss Outdated Show resolved Hide resolved
src/css/wizard-navigation-bar.scss Outdated Show resolved Hide resolved
src/css/wizard-navigation-bar.scss Show resolved Hide resolved
src/css/wizard-navigation-bar.scss Show resolved Hide resolved
src/css/wizard-navigation-bar.scss Show resolved Hide resolved
earshinov added a commit to earshinov/angular-archwizard-demo that referenced this pull request Sep 23, 2019
@earshinov
Copy link
Contributor Author

Please bear with some of my more silly questions. I'm not that knowledgeable with SCSS, therefore I've added some questions about parts of the code that I don't (fully) understand.

That's good. I think it is essential to make the code understandable to the project maintainer at the very least :-) I have added a lot of comments. Please review.

Copy link
Owner

@madoar madoar left a comment

Choose a reason for hiding this comment

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

In addition to the following comments I have two requests:

  • the README file needs to be changed to reflect the changes done in this PR
  • your introduced variables start with $wz-... I guess wz stands for wizard right? Wouldn't it be better and more in line with our other definitions if the variables start with $aw (i.e. archwizard)?

src/css/archwizard.scss Outdated Show resolved Hide resolved

// Color definitions
//
// This is a mapping from indicator style and wizard step state to colors.
Copy link
Owner

Choose a reason for hiding this comment

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

This as in the color definitions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In 6202716 I squashed the two lines into

// Color definitions - a mapping from indicator style and wizard step state to colors.

This way it is more concise.

src/css/archwizard.scss Show resolved Hide resolved
src/css/archwizard.scss Outdated Show resolved Hide resolved
@earshinov
Copy link
Contributor Author

Your introduced variables start with $wz-... I guess wz stands for wizard right? Wouldn't it be better and more in line with our other definitions if the variables start with $aw (i.e. archwizard)?

I just followed the convention I saw in *.less files where variables had $wz-... prefix as well.

I agree that '$aw' is more appropriate. Let's rename.

Also, I would rename 'dots' to 'circles'.

Copy link
Owner

@madoar madoar left a comment

Choose a reason for hiding this comment

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

I just followed the convention I saw in *.less files where variables had $wz-... prefix as well.

I agree that '$aw' is more appropriate. Let's rename.

The styles are really old. In addition in the first version of the wizard I used the styles from https://github.com/angular-wizard/angular-wizard/ so it is quite possible that the names are from there.

Also, I would rename 'dots' to 'circles'.

Good idea!

I'll test the PR later today.

I think I prefer to add this PR to a new major release (i.e. 6.0.0) because it can lead to breaking changes for users with custom styles. @earshinov what do you suggest?

README.md Outdated Show resolved Hide resolved
src/css/wizard-navigation-bar.scss Show resolved Hide resolved
@earshinov
Copy link
Contributor Author

I think I prefer to add this PR to a new major release (i.e. 6.0.0) because it can lead to breaking changes for users with custom styles. @earshinov what do you suggest?

Yes, I think this change is worth a new major version.

@madoar madoar merged commit 2db7bd2 into madoar:develop Oct 6, 2019
@earshinov earshinov deleted the advanced-styles-customization branch October 13, 2019 13:35
madoar pushed a commit to madoar/angular-archwizard-demo that referenced this pull request Oct 20, 2019
* Update style customization demo on the 'Free navigation mode' page for the 'advanced-styles-customization' branch of angular-archwizard
* Add "Wizard with single step" demo page
   Discussion: madoar/angular-archwizard#235 (comment)
* Adjust custom styles for wz- prefix renamed into aw-
* 'Custom global SCSS' demo (experimental)
* Split the single-step example into two, one for horizontal and one for vertical layout
* Merge single-step examples into a single group in the sidebar
* Add an elaborate description to the 'Custom SCSS' demo page
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants