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: add optional color for slide-toggle when unactivated #90

Closed

Conversation

willdp
Copy link
Contributor

@willdp willdp commented Jan 17, 2020

add optional color for slide0toggle when it is unactivated, also created
storybook for each case

add optional color for slide0toggle when it is unactivated, also created
storybook for each case
{
cod: "3",
checked: false,
title: "UNACTIVATE COLOR",
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the title be 'Inactive'?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe 'unchecked', to keep consistency with component property

@@ -9,6 +9,7 @@ import { FormControl } from "@angular/forms";
export class SlideToggleComponent implements OnInit {
checked = true;
colorFormControl = new FormControl("#08B25A");
uncheckedColorFormControl = new FormControl("#000000")
Copy link
Contributor

Choose a reason for hiding this comment

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

missing ;

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, please add the access modifier for these properties

type="color"
placeholder="Unchecked Color"
[formControl]="uncheckedColorFormControl"
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

/> could be moved to the previous line

[title]="toggle?.title"
(toggleChange)="toggleChange(toggle?.cod, $event)"
ngDefaultControl
></gor-slide-toggle>
</div>
</ng-container>
</div>
</ng-container>
</ng-container>
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing newline at end of file

<span class="slider-before" [ngClass]="{ active: checked }"></span>
</span>
</label>
<span class="title">{{ title }}</span>
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing newline at end of file

* @Optional
*/
color?: string;


Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove extra empty line

)
.addDecorator(withKnobs)
.add(
'three different cases about slide-toggle',
Copy link
Contributor

Choose a reason for hiding this comment

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

Summary should be descriptive of functionality being demonstrated - in this case, color configurations

storiesOf('slide-toggle', module)
.addDecorator(
moduleMetadata({
imports: [CommonModule],
Copy link
Contributor

Choose a reason for hiding this comment

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

Check if this import is necessary

@@ -0,0 +1,74 @@
import { storiesOf, moduleMetadata } from '@storybook/angular';
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice stories, keep up the good work

checked: false,
title: "UNACTIVATE COLOR",
color: "",
uncheckedColor: "#456888"
Copy link
Contributor

@oliveira-gust oliveira-gust Jan 17, 2020

Choose a reason for hiding this comment

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

Check code identation in all this file

{
cod: "3",
checked: false,
title: "UNACTIVATE COLOR",
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep text capitalization consistent across titles

>
</gor-slide-toggle>
`
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing newline at end of file

fix some sintax and identation erros, also add acess modifier for some
properties
.prettierignore Outdated
@@ -4,3 +4,4 @@
**/*.md
**/*.scss
**/*.html
**/*.stories.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing newline at end of file

public colorFormControl = new FormControl('#08B25A');
public uncheckedColorFormControl = new FormControl('#000000');
public disabled = false;
public titleFormControl = new FormControl('click me');

constructor() {}

ngOnInit() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Empty methods can be removed

styleUrls: ["./slide-toggle.component.scss"]
selector: 'gorilainvest-slide-toggle',
templateUrl: './slide-toggle.component.html',
styleUrls: ['./slide-toggle.component.scss']
})
export class SlideToggleComponent implements OnInit {
Copy link
Contributor

Choose a reason for hiding this comment

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

OnInit interface can be removed

{
cod: '3',
checked: false,
title: 'UNCHEKED COLOR',
Copy link
Contributor

Choose a reason for hiding this comment

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

Please correct typo in title

const labels = ['title'];

return {

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove extra empty line

[labels]='${JSON.stringify(labels)}'
>
</gor-slide-toggle-group>
`
Copy link
Contributor

Choose a reason for hiding this comment

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

Check identation

Copy link
Contributor

@guilhermejcgois guilhermejcgois left a comment

Choose a reason for hiding this comment

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

Nice job, thanks for this new feature! Just one little thing before we can go... Can you check story name? We don't use the component selector to name stories... Also, can you too check the plural of examples titles?

@guilhermejcgois
Copy link
Contributor

Two more things before merge the PR:

  1. Fix last commits author
  2. Update branch

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

4 participants