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(Tabs): implementa la componente dei Tab #65

Merged
merged 13 commits into from
Aug 30, 2018
Merged

Conversation

ciccio86
Copy link
Contributor

Descrizione

Implementata la componente dei Tab

Checklist

@ciccio86 ciccio86 requested a review from francescozaia August 20, 2018 15:02
Copy link
Member

@francescozaia francescozaia left a comment

Choose a reason for hiding this comment

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

Niente di grave, solo typo o mie curiosità. 👍 😄

@@ -0,0 +1,7 @@
.nav-link {
Copy link
Member

Choose a reason for hiding this comment

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

Sono solo curioso: come mai hai dovuto sovrascrivere questi stili?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dovrebbe essere un retaggio di una prova precedente in cui avevo tolto l'href dal link (con conseguente problema negli stili). Successivamente l'ho risolta con un preventDefault().
In breve non appena ho 5 minuti correggo.

import { TabComponent } from './tab.component';

/** Usato per generare ID univoci per ogni componente tab */
let nextId = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Si potrebbe usare uno uuid per questi?

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 tutti gli altri componenti abbiamo usato un id incrementale, inoltre in questo modo ci è più comodo riferirci ad un componente durante gli e2e test. Sarebbe un po' difficile nel caso di uuid.
Questo però mi fa venire in mente un'altra cosa: forse dovremmo, oltre ad autoassegnare l'id alla componente creata, dare la possibilità all'utente di passare un proprio id.

/**
* Un componente tab-group con design bootstrap italia. Utilizzabile con il tag `<it-tab-group>`.
*
* Supporta al suo interno tab di base ``<it-tab> con una label e un contenuto.
Copy link
Member

Choose a reason for hiding this comment

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

mini-typo nel commento <it-tab>


/**
* Riferimento all'elemento dal quale il tab è etichettato.
* Viene azzerto se `aria-label` è impostato.
Copy link
Member

Choose a reason for hiding this comment

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

Viene azzerato (o meglio, resettato) se ...

}

static _isNumberValue(value: any): boolean {
return !isNaN(parseFloat(value as any)) && !isNaN(Number(value));
Copy link
Member

Choose a reason for hiding this comment

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

Sono curioso scusa: && !isNaN(Number(value) è necessario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ho preso questa riga di codice da qui:
https://github.com/angular/material2/blob/master/src/cdk/coercion/number-property.ts

Pare che non gestisca stringhe del tipo 123hello

Copy link
Member

Choose a reason for hiding this comment

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

Chiaro, grazie

@francescozaia
Copy link
Member

francescozaia commented Aug 29, 2018

Tutto ok @ciccio86 grazie anche per le spiegazioni e perdona il ritardo nella review!
Mergiabile quando volete.

@ciccio86 ciccio86 merged commit cb4a810 into master Aug 30, 2018
@td-machineuser
Copy link
Collaborator

🎉 This PR is included in version 0.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ciccio86
Copy link
Contributor Author

Fix #47

@ciccio86 ciccio86 deleted the feature/47-tabs branch September 17, 2018 08:27
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.

3 participants