Refactor styles #161

earshinov · 2018-10-17T16:49:32Z

A styles refactoring proposal as a follow-up to #159 (review).

This PR is on top of #159.

…er-width`

madoar

I like the approach of this PR a lot!

I believe the code needs some additional comments to help the reader understand what exactly happens happens and how the different mixins function together.
For example the application of .define-state(...) can be hard to understand.

Shall I merge #159 separately?

madoar · 2018-10-18T14:51:57Z

src/lib/components/wizard-navigation-bar.component.vertical.less

-  .state-circle-with-border-hover(@dot-border-width, @circle-color) {
-    border-color: darken(@circle-color, 10%);
+
+  li .step-indicator {


I'm in favor of adding this and the following blocks to the previous li block:

li { .step-indicator { ... } &.current .step-indicator { ... } ... }

I think this makes the code easier to read and understand.

earshinov · 2018-10-20T14:10:10Z

@madoar,

I believe the code needs some additional comments to help the reader understand what exactly happens happens and how the different mixins function together.
For example the application of .define-state(...) can be hard to understand.

Yes, definitely. I did not write any comments in the first place because I was not sure if you would favor my approach.

Shall I merge #159 separately?

Yes, I think it should not be blocked by this refactoring.

… more appropriate.

earshinov · 2018-10-20T15:21:10Z

Added code comments in 39b7430. If everything is OK, I can then apply the same refactoring procedure to the styles for horizontal layout.

madoar · 2018-10-21T09:41:04Z

src/lib/components/wizard-navigation-bar.component.vertical.less

+// Arguments:
+//   @width  - step indicator width
+//   @height - step indicator height
+//   @border - step indicator border


I think the arguments need some more description.
For example, the @border argument description doesn't specify what information the argument needs to contain:

whether a border should be drawn (i.e. a boolean)

how thick a border should be drawn

or something entirely different?

madoar · 2018-10-21T09:43:39Z

src/lib/components/wizard-navigation-bar.component.vertical.less

 @wz-color-default: #E6E6E6;
 @wz-color-current: #808080;
 @wz-color-done: #339933;
 @wz-color-optional: #38ef38;
 @wz-color-editing: #FF0000;

 /*
- dot definitions
- */
+dot definitions


I know this isn't really a part of this PR, but can you please change the style of the old comments:

/* ... */

to fit with the new ones:

// ...

madoar · 2018-10-21T09:47:36Z

src/lib/components/wizard-navigation-bar.component.vertical.less

-          .state-circle-hover(@big-dot-width, @big-dot-height, @dot-border-width);
-          .state-circle-with-border-and-content-hover(@dot-border-width, @wz-color-default);
-        }
+  &.small ul.steps-indicator {


Sometimes, a user of the library may want to completely override the style of the library, i.e. he doesn't want to use any existing style but define his own one.
In such a case it would be useful to define a kind of "empty" default style.

I'm not sure how this could be done best, and whether this fits in the scope of this PR.
My current approach to such questions was to recommend using a non existing layout name in [navBarLayout], but I'm not sure if this is the best approach.
What do you think about this?

My first thought was that providing a default style is a good and easily implemented idea:

ul.steps-indicator { .state-default(@color) { } .state-hover(@color) { } .state-nohover(@color) { } .define-style(/* ??? */); }

...but we will then have to decide which indicator width, height and border-width users are going to need. There will always be users unsatisfied with any assumption we make, so I'd rather not provide any defaults at all to avoid such questions.

madoar · 2018-10-21T09:52:01Z

src/lib/components/wizard-navigation-bar.component.vertical.less

-          .line(@big-dot-width, @big-dot-height, @wz-color-default);
-        }
+  // default steps shouldn't change when hovered, because they aren't clickable
+  li.default a:hover .step-indicator {


Sometimes we're only selecting a css class, e.g. .step-indicator, while we're some other times selecting an element and a css class, e.g. ul.steps-indicator.
I think it would be best if we try to always use a combination of an element and a css class, because it makes it easier to identify on which element type a css class is applied.
What do you think?

I think it would be best if we try to always use a combination of an element and a css class, because it makes it easier to identify on which element type a css class is applied.

True, unless the element type is div or span, which carry little meaning. I tend to think of div's and span's as placeholders to which some meaning is assigned by way of CSS classes, so adding div or span to CSS selectors, in my opinion, has little sense. The code is good as it is :)

earshinov · 2018-10-21T13:14:41Z

@madoar , I have pushed another set of changes. Please review.

madoar · 2018-10-21T17:20:32Z

I'm ok with merging this PR.
Do you want to append the changes for the horizontal layouts to this PR, or do you want to create a new one for them?

earshinov · 2018-10-21T17:44:04Z

I'd rather add changes for the horizontal layout to this PR. I think I'll find time for it and #166 later today or tomorrow.

earshinov · 2018-10-22T12:07:15Z

Done, please review.

…and `abstract class BaseNavigationMode`.

earshinov · 2018-10-22T14:56:14Z

Sorry, I was absent-minded and committed to a wrong branch. Reverted.

@ViewChild

* master: - adjust version - add angular version 7 to the keywords - fix badges in README Update to angular 7 (madoar#194) Update the badges (madoar#193) Cleanup .travis.yml (madoar#191) Remove unnecesarry index.ts files (madoar#192) Apply the [stepId] field from the WizardSteps at ids in the navigation bar (madoar#186) Update the index.ts files (madoar#187) Removing barrel export and explicitly exporting each component in base index.ts. Barrel exports of components cause @ViewChild(<className>) to give undefined in AOT builds (madoar#184) Declare Angular 7 compatibility in `package.json` (madoar#165) Refactor styles (madoar#161) Restore vertical nav bar layout, in particular label alignment (madoar#159)

earshinov added 3 commits October 14, 2018 19:32

Restore vertical nav bar layout, in particular label alignment

35e0019

Further adjust label alignment in vertical layout: remove `+ 2 * bord…

388a977

…er-width`

Experimental styles refactoring (for now for vertical layout only)

941775a

earshinov mentioned this pull request Oct 17, 2018

Restore vertical nav bar layout, in particular label alignment #159

Merged

madoar reviewed Oct 18, 2018

View reviewed changes

earshinov added 2 commits October 20, 2018 17:53

Minor cleanup: Nest style definitions

6370d56

Document .define-state mixin, rename to .define-style which seems…

39b7430

… more appropriate.

madoar reviewed Oct 21, 2018

View reviewed changes

earshinov added 3 commits October 21, 2018 15:34

Cleanup: Rename @border mixin argument to @border-width for clarity

bfa1d63

Convert comments from /* to // in *.less files on the way

f982afc

Merge branch 'develop' to resolve conflicts

9a0da18

earshinov added 2 commits October 22, 2018 14:51

Apply the same refactoring to horisontal layout

1efcb5b

Fix: Define @border-width: 0 for the large-filled-symbols style

ae8fd0f

earshinov added 2 commits October 22, 2018 16:32

Split abstract class NavigationMode into interface NavigatinMode …

eee01f4

…and `abstract class BaseNavigationMode`.

Revert eee01f4.

0f367ad

madoar merged commit f889758 into madoar:develop Oct 23, 2018

earshinov deleted the refactor-styles branch December 24, 2018 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor styles #161

Refactor styles #161

earshinov commented Oct 17, 2018 •

edited

Loading

madoar left a comment

madoar Oct 18, 2018

earshinov Oct 20, 2018

earshinov commented Oct 20, 2018

earshinov commented Oct 20, 2018

madoar Oct 21, 2018

earshinov Oct 21, 2018

madoar Oct 21, 2018

earshinov Oct 21, 2018

madoar Oct 21, 2018

earshinov Oct 21, 2018

madoar Oct 21, 2018

earshinov Oct 21, 2018

earshinov commented Oct 21, 2018

madoar commented Oct 21, 2018

earshinov commented Oct 21, 2018

earshinov commented Oct 22, 2018

earshinov commented Oct 22, 2018

Refactor styles #161

Refactor styles #161

Conversation

earshinov commented Oct 17, 2018 • edited Loading

madoar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

earshinov commented Oct 20, 2018

earshinov commented Oct 20, 2018

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

earshinov commented Oct 21, 2018

madoar commented Oct 21, 2018

earshinov commented Oct 21, 2018

earshinov commented Oct 22, 2018

earshinov commented Oct 22, 2018

earshinov commented Oct 17, 2018 •

edited

Loading