Skip to content
This repository has been archived by the owner on Mar 10, 2022. It is now read-only.

(pC-1876) timezoned date field #326

Merged
merged 9 commits into from
May 15, 2019
Merged

Conversation

Ledoux
Copy link
Contributor

@Ledoux Ledoux commented May 9, 2019

No description provided.

@Ledoux Ledoux changed the title [WIP] (pC-1876) timezoned date field (pC-1876) timezoned date field May 9, 2019
@@ -60,6 +66,9 @@ export const DateField = ({
let selected
Copy link
Contributor

Choose a reason for hiding this comment

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

j'extrairais la fonction donnée au render en la nommant.
Je ne comprends pas ce qu'est readOnlyValue ni selected à la lecture de leur nom : pourrait-on leur donner un nom plus parlant ? J'ai l'impression que ce sont des dates sous différents formats, à mon niveau de compréhension je peux te proposer datetime et formattedDateTime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, j'ai renommé en formattedSelectedDatetime et selectedDatetime.
Pour extraire et nommer la fonction render, je suis moins d'accord je trouverais ca moins lisible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personnellement je n'arrive pas à comprendre ce que fait la fonction du render et je suis incapable de dire si c'est parce que je ne suis pas assez à l'aise en REACT ou parce que j'ai besoin d'un nommage pour me retrouver.

Il me semble que c'est plus une question "pratiques de code" ici et il me semblait que le nommage de fonctions anonymes en faisait partie. Mais comme je suis moins calée sur le front, je ne sais pas trop comment trancher sur ce point.

Copy link
Contributor Author

@Ledoux Ledoux May 15, 2019

Choose a reason for hiding this comment

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

yes, je comprends. Les arguments pour defendre mon point sont:

  • s'il fallait écrire la fonction render={fieldProps => { const props=f(DateFieldProps, fieldProps); return renderField(props)}}, il faudrait d'une part écrire cette fonction plus haut dans le code, ce qui perd en lecture directe de l'arbre html de tout le composant (qui dans ce cas n'est pas trop gros, comme dans tous les render des autres components de type Field).
  • il faut savoir écrire cette fonction const props=f(DateFieldProps, fieldProps) qui à ma sensation, est de la complexité pour pas grand chose. Donc dans mon estimation, l'augmentation de la surface d'api dans ce cas nuit plus à la lisibilité et l'expressivité du composant.
  • comme c'est un component qui ne touche pas encore au metier de l'appli, dur de trouver un autre nom que renderField (à moins que tu aies une idée).

Dans ce cas donc, j'aimerais tenir aussi ma position. Je propose qu'on demande un troisième avis qui tranche pour nous.

src/components/layout/form/fields/tests/DateField.spec.jsx Outdated Show resolved Hide resolved
@Aliochka Aliochka self-requested a review May 10, 2019 13:02
Copy link
Contributor

@Aliochka Aliochka left a comment

Choose a reason for hiding this comment

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

Petite question un peu plus générale sur les timzone :
on les construits en fonction du department du code du lieu,
'Europe/Paris' s'il n'y en a pas,

j'imagine que c'est aussi ce qu'il se passe pour les lieux numériques,

du coup est-ce que ton problème ne se reposera pas dans le cas d'un offreur numérique en Outre-Mer ?
(dans ce cas il n'y a que des bokingLimiltDatetime)

@Ledoux
Copy link
Contributor Author

Ledoux commented May 13, 2019

@Aliochka very good point!
De ce qu'on a décidé donc:

  • on va faire un selectTimezone qui cherche celui du lieu pour le cas non virtuel et celui de la structure pour le cas virtuel.
  • on rajoute un tooltip pour bien rappeler dans le cas vrituel qu'il s'agit de la timezone de la structure correspondante.

@Ledoux Ledoux force-pushed the pc-1876-fix-timezone-in-datefield branch from 18130fe to 1655009 Compare May 13, 2019 17:16
@Ledoux Ledoux force-pushed the pc-1876-fix-timezone-in-datefield branch from 023ca30 to c179c73 Compare May 14, 2019 08:24
@Ledoux Ledoux force-pushed the pc-1876-fix-timezone-in-datefield branch from c179c73 to 076e79a Compare May 14, 2019 08:42
@Ledoux Ledoux force-pushed the pc-1876-fix-timezone-in-datefield branch from 92a96cd to bf42096 Compare May 14, 2019 09:37
@Ledoux
Copy link
Contributor Author

Ledoux commented May 14, 2019

@sofcalca ok merci, j'ai fait mes retours, quand tu as le temps, pourras-tu confirmer la PR si elle te convient, merci:)

@Ledoux Ledoux requested review from Akhilian and atusithi May 15, 2019 09:41
}

let tip =
"L'heure limite de réservation est à 23h59 de ce jour et ne peut pas être changée."
Copy link
Contributor

Choose a reason for hiding this comment

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

Renommer "L'heure limite de réservation est 23h59 ce jour et ne peut pas être changée."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

let tip =
"L'heure limite de réservation est à 23h59 de ce jour et ne peut pas être changée."
if (venue && venue.isVirtual) {
tip = `${tip}<br /><br /> Vous être sur une offre numérique, la zone horaire de cette date correspond à celle de votre structure.`
Copy link
Contributor

@atusithi atusithi May 15, 2019

Choose a reason for hiding this comment

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

Pourquoi un double saut de ligne via
?
Renommer la phrase : Vous êtes sur une offre numérique, la zone horaire de cette date correspond à celle de votre structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2019-05-15 at 17 13 25

Copy link
Contributor Author

Choose a reason for hiding this comment

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

le saut de ligne pour avoir ça:)

bindTimeFieldWithDateField({
dateName: 'beginningDatetime',
timeName: 'beginningTime',

Copy link
Contributor

Choose a reason for hiding this comment

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

supprimer saut de ligne

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

import HeroSection from 'components/layout/HeroSection'

const getStocksManagerButtonTitle = (isEvent, stocks) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Pourquoi on sort cette fonction de la class définissant le composant ? Elle n'a pas l'air d'être utilisée à l'extérieur et n'a de sens que dans le composant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

price: 17,
}

describe.skip('src | components | pages | Offer | StocksManager', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

si le test est skip, on le supprime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 on garde juste le snapshot

return `${dateName || ''}${hours || ''}${minutes || ''}${timezone || ''}`
}

const forceDateAtSpecificHoursAndMinutes = createCachedSelector(
Copy link
Contributor

Choose a reason for hiding this comment

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

renommer en forceDateTimeAtSpecificHoursAndMinutes ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

dateMoment = dateMoment.tz(timezone)
}

const updatedDate = dateMoment
Copy link
Contributor

Choose a reason for hiding this comment

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

renommer en updatedDateTime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

({ timezone }) => timezone,
(dateName, hours, minutes, timezone) =>
createDecorator({
field: dateName,
Copy link
Contributor

Choose a reason for hiding this comment

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

dateTimeName ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

// then
expect(result).toEqual('Europe/Paris')
})
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Il manque un test : le cas où l'offerer est undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@Ledoux
Copy link
Contributor Author

Ledoux commented May 15, 2019

@Akhilian j'ai fait les retours tu me confirmes ? merci

@Ledoux Ledoux merged commit a935c41 into master May 15, 2019
@Ledoux Ledoux deleted the pc-1876-fix-timezone-in-datefield branch May 15, 2019 16:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants