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

feat(checkbox): Eva style 💅 #1311

Merged
merged 47 commits into from
Apr 2, 2019
Merged

feat(checkbox): Eva style 💅 #1311

merged 47 commits into from
Apr 2, 2019

Conversation

yggg
Copy link
Contributor

@yggg yggg commented Mar 21, 2019

Please read and mark the following check list before creating a pull request:

Short description of what this resolves:

Adds Eva theme variables to theme file.
Restyle checkbox in Eva styles. Add indeterminate state, 'change' output event and two new statuses (primary, info).

BREAKING CHANGES:
Following map keys were removed:

  • border-color
  • color-gray
  • color-neutral
  • color-disabled
    Mapping for borders, disabled and similar styles would controlled
    by design system and configured for each component individually.

Properties 'checkbox-bg', 'checkbox-checked-bg' and 'checkbox-disabled-bg'
replaced with:

  • checkbox-disabled-background-color
  • checkbox-[status]-background-color
  • checkbox-[status]-checked-background-color
  • checkbox-[status]-indeterminate-background-color
  • checkbox-[status]-focus-background-color
  • checkbox-[status]-hover-background-color
  • checkbox-[status]-active-background-color

Property 'checkbox-size' splited into 'checkbox-height', 'checkbox-width'.

Following properties were removed (along with ability to change appearance of checkbox
based on checked or disabled state):

  • checkbox-checked-size
  • checkbox-checked-border-size
  • checkbox-disabled-size
  • checkbox-disabled-border-size

Property 'checkbox-border-size' replaced with 'checkbox-border-width'.

Properties 'checkbox-border-color', 'checkbox-checked-border-color', 'checkbox-disabled-border-color'
replaced with:

  • checkbox-disabled-border-color
  • checkbox-[status]-border-color
  • checkbox-[status]-checked-border-color
  • checkbox-[status]-indeterminate-border-color
  • checkbox-[status]-hover-border-color
  • checkbox-[status]-active-border-color

Properties 'checkbox-checkmark', 'checkbox-checked-checkmark', 'checkbox-disabled-checkmark'
replaced with:

  • checkbox-disabled-checkmark-color
  • checkbox-[status]-checked-checkmark-color
  • checkbox-[status]-indeterminate-checkmark-color

Check mark pseudo element replaced with nb-icon.

Checkbox classes were changed.
Class 'customised-control' replaced with 'label'.
Class 'customised-control-input' replaced with 'native-input'.
Class 'customised-control-indicator' replaced with 'custom-checkbox'.
Class 'customised-control-description' replaced with 'text'.

NbCheckboxComponent's '_value' property now private. Use 'value' instead.

@yggg yggg marked this pull request as ready for review March 22, 2019 07:46
@yggg yggg requested a review from nnixaa March 22, 2019 07:46
width: nb-theme(checkbox-size);
height: nb-theme(checkbox-size);
border: nb-theme(checkbox-border-size) solid nb-theme(checkbox-border-color);
$status-variants: 'primary', 'success', 'warning', 'danger', 'info', 'white';
Copy link
Collaborator

Choose a reason for hiding this comment

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

so will move this into a mixin one day, right?

border-width: 0 $border-size $border-size 0;
}
}
svg {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we would need to use nb-icon right after we merge it

nb-theme(checkbox-border-size)
);
use {
fill: nb-theme(checkbox-#{$status}-checked-checkmark-color);
Copy link
Collaborator

Choose a reason for hiding this comment

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

and then we can just specify color to style the icon

@@ -0,0 +1,8 @@
export enum NbCheckboxStatus {
Copy link
Collaborator

Choose a reason for hiding this comment

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

and this would be NbComponentStatus I believe? So that we can share it across the component.

@@ -91,27 +211,56 @@ export class NbCheckboxComponent implements ControlValueAccessor {
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's also remove all named inputs and use _ as a prefix for private vars, as _disabled.

@yggg yggg changed the base branch from feat/4.0 to next March 30, 2019 15:24
@codecov
Copy link

codecov bot commented Mar 30, 2019

Codecov Report

Merging #1311 into next will increase coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##             next    #1311      +/-   ##
==========================================
+ Coverage   81.37%   81.47%   +0.09%     
==========================================
  Files         237      237              
  Lines        7212     7233      +21     
  Branches      617      618       +1     
==========================================
+ Hits         5869     5893      +24     
+ Misses       1157     1154       -3     
  Partials      186      186
Impacted Files Coverage Δ
src/framework/theme/components/icon/icon.module.ts 81.81% <ø> (ø) ⬆️
...rk/theme/components/checkbox/checkbox.component.ts 100% <100%> (+8.33%) ⬆️
...ework/theme/components/checkbox/checkbox.module.ts 100% <100%> (ø) ⬆️

nnixaa and others added 23 commits April 1, 2019 18:55
…-icons package (#1319)

BREAKING CHANGE:

Starting from version 4.0, Nebular introduces new `nb-icon` component and `NbIconLibraries` service to host SVG and Font icon packs. As a breaking change, Nebular moves from `nebular-icons` package to much more popular [Eva Icons pack](https://akveo.github.io/eva-icons/) consisting of 480+ beautiful SVG icons. We believe this will bring more quality and variety to interfaces based on Nebular.

Now all Nebular components internally use `<nb-icon></nb-icon>` component utilizing Eva Icons SVG icons. More details on [nb-icon](https://akveo.github.io/nebular/docs/components/icon) component.

There are two ways to upgrade:
**Migrate to Eva Icons** (recommended):
1) install Eva Icons Nebular package `npm i @nebular/eva-icons`
2) register `NbEvaIconsModule` in the `app.module.ts`
```
import { NbEvaIconsModule } form '@nebular/eva-icons';

@NgModule({
  imports: [
  	// ...
    NbEvaIconsModule,
  ],
})
```
3) Search for all usages of `<span icon="nb-*"` or ``<i icon="nb-*"``and replace with `<nb-icon icon="icon-name"></nb-icon>`. Full icons list https://akveo.github.io/eva-icons/.

4) Search for `icon: 'nb-*'` references in properties for such components as Menu, Actions, Tabs, etc. Replace those with `icon: 'icon-name'`. Please note, there is no need to specify any icon prefix (such as `eva-` or `nb-`) since prefix is specified when the icon package is registered in Nebular. 

4) Update styles if necessary.

5) if you have `nebular-icons` installed, remove the package and all references.

**Continue using nebular-icons**
This option is also possible, but please note, Nebular Component will still use Eva Icons pack for internal component icons, such as `close`, `arrow-down`, `arrow-up`, etc.

1) Register nebular-icons as a pack for Nebular in your `app.component.ts`
```
  import { NbIconLibraries } from '@nebular/theme';

  constructor(private iconLibraries: NbIconLibraries) {
    this.iconLibraries.registerFontPack('nebular', { iconClassPrefix: 'nb' });
    this.iconLibraries.setDefaultPack('nebular');
  }
```

3) Search for all usages of `<span icon="nb-*"` or ``<i icon="nb-*"`` and replace with `<nb-icon icon="icon-name"></nb-icon>` without the `nb-` prefix  since prefix is specified when the icon package is registered in Nebular. 

4) Search for `icon: 'nb-*'` references in properties for such components as Menu, Actions, Tabs, etc. Replace those with `icon: 'icon-name'` without `nb-` prefix since it is unnecessary and covered under the hood.

Please open an issue if you have any questions or having difficulties to migrate.
BREAKING CHANGE:
Following map keys were removed:
  - border-color
  - color-gray
  - color-neutral
  - color-disabled
Mapping for borders, disabled and similar styles would controlled
by design system and configured for each component individually.
BREAKING CHANGE:
Properties 'checkbox-bg', 'checkbox-checked-bg' and 'checkbox-disabled-bg'
replaced with:
  - checkbox-disabled-background-color
  - checkbox-[status]-background-color
  - checkbox-[status]-checked-background-color
  - checkbox-[status]-indeterminate-background-color
  - checkbox-[status]-focus-background-color
  - checkbox-[status]-hover-background-color
  - checkbox-[status]-active-background-color

Property 'checkbox-size' splited into 'checkbox-height', 'checkbox-width'.

Following properties were removed (along with ability to change appearance of checkbox
based on checked or disabled state):
  - checkbox-checked-size
  - checkbox-checked-border-size
  - checkbox-disabled-size removed
  - checkbox-disabled-border-size

Property 'checkbox-border-size' replaced with 'checkbox-border-width'.

Properties 'checkbox-border-color', 'checkbox-checked-border-color', 'checkbox-disabled-border-color'
replaced with:
  - checkbox-disabled-border-color
  - checkbox-[status]-border-color
  - checkbox-[status]-checked-border-color
  - checkbox-[status]-indeterminate-border-color
  - checkbox-[status]-hover-border-color
  - checkbox-[status]-active-border-color

Properties 'checkbox-checkmark', 'checkbox-checked-checkmark', 'checkbox-disabled-checkmark'
replaced with:
  - checkbox-disabled-checkmark-color
  - checkbox-[status]-checked-checkmark-color
  - checkbox-[status]-indeterminate-checkmark-color
BREAKING CHANGE:
Description for change made in 37a490b.

Check mark pseudo element replaced with svg.

Checkbox classes were changed.
Class 'customised-control' replaced with 'label'.
Class 'customised-control-input' replaced with 'native-input'.
Class 'customised-control-indicator' replaced with 'custom-checkbox'.
Class 'customised-control-description' replaced with 'text'.
package.json Show resolved Hide resolved
nb-theme(checkbox-checked-checkmark),
nb-theme(checkbox-checked-border-size)
);
transition-duration: nb-theme(checkbox-state-transition-duration);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking we should use Angular animations rather than css

);
}
@mixin nb-checkbox-status($status: '') {
$status: if($status == '', '', -#{$status});
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably we should not have the - part in here -#{$status}
and leave it there

background-color: nb-theme(checkbox-#{$status}-background-color);

for redability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nb-checkbox-status called without a value (to set default styling) so -#{$status} used to prevent extra dash in variables name when $status is ''. But I agree it looks bad, will try to refactor.

@@ -83,15 +83,15 @@
}

nb-checkbox {
.customised-control-description {
Copy link
Collaborator

Choose a reason for hiding this comment

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

select styles in checkbox PR?

@@ -269,3 +269,31 @@ $nb-themes-export: () !global;
}
}
}

@mixin nb-install-global() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we have new nb-install-global here?

@yggg yggg requested a review from nnixaa April 2, 2019 09:14
@yggg yggg merged commit 9a7ecad into akveo:next Apr 2, 2019
brannon-darby pushed a commit to brannon-darby/nebular that referenced this pull request Apr 5, 2019
brannon-darby pushed a commit to brannon-darby/nebular that referenced this pull request Apr 5, 2019
yggg added a commit that referenced this pull request Apr 17, 2019
yggg added a commit that referenced this pull request May 27, 2019
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.

2 participants