Fixes #21312 - components for formatting dates #4909

tstrachota · 2017-10-12T12:27:39Z

This patch reuses components from react-intl and extends the component registry to provide an intl wrapper (react-intl requires the component to be wrapped in IntlProvider). That could be used also if we decide that react-intl is the way to for translating our whole react stack.

TODO:

tests for the components

theforeman-bot · 2017-10-12T12:27:45Z

Issues: #21312

houndci-bot · 2017-10-12T12:27:52Z

webpack/assets/javascripts/react_app/components/compontentRegistry.test.js

@@ -52,7 +52,7 @@ describe('Component registry', () => {
    const name = 'MarkupComponent';

    componentRegistry.register({ name, type: FakeComponent, store: false });
-    const markup = componentRegistry.markup(name, { fakeData: true }, {});
+    const markup = componentRegistry.markup(name, { data: { fakeData: true }, store: {}, wrapWithIntl: false });


Line 55 exceeds the maximum line length of 100 max-len

houndci-bot · 2017-10-12T12:27:53Z

webpack/assets/javascripts/react_app/components/componentRegistry.js

-  { name: 'StorageContainer', type: StorageContainer }
+  { name: 'StorageContainer', type: StorageContainer },
+  { name: 'RelativeDate', type: RelativeDate, data: false, store: false },
+  { name: 'AbsoluteDate', type: AbsoluteDate, data: false, store: false },


Unexpected trailing comma comma-dangle

houndci-bot · 2017-10-12T12:27:53Z

webpack/assets/javascripts/react_app/components/componentRegistry.js

+  getType(wrapWithIntl = true) {
+    if (wrapWithIntl) {
+      return wrapWithIntlProvider(this.type);
+    } else {


Unnecessary 'else' after 'return' no-else-return

houndci-bot · 2017-10-12T12:27:53Z

webpack/assets/javascripts/react_app/components/componentRegistry.js

 import StorageContainer from './hosts/storage/vmware/';
 import forEach from 'lodash/forEach';
 import map from 'lodash/map';
+import { wrapWithIntlProvider } from '../common/i18n'


Missing semicolon semi

houndci-bot · 2017-10-12T12:27:53Z

webpack/assets/javascripts/react_app/components/common/Date/RelativeDate.js

+
+RelativeDate.propTypes = {
+  date: PropTypes.any.isRequired,
+  default: PropTypes.string,


Unexpected trailing comma comma-dangle

houndci-bot · 2017-10-12T12:27:53Z

webpack/assets/javascripts/react_app/components/common/Date/AbsoluteDate.js

+      return (
+        <span title={title}>
+          <FormattedDate value={date}
+            day='2-digit'


Unexpected usage of singlequote jsx-quotes

houndci-bot · 2017-10-12T12:27:54Z

webpack/assets/javascripts/react_app/common/i18n.js

+import React from 'react';
+import {injectIntl, IntlProvider, addLocaleData} from 'react-intl';
+
+export var locale = document.getElementsByTagName('html')[0].lang || 'en';


Unexpected var, use let or const instead no-var

houndci-bot · 2017-10-12T12:27:54Z

webpack/assets/javascripts/react_app/common/MountingService.js

@@ -7,7 +7,7 @@ export function mount(component, selector, data) {

  if (reactNode) {
    ReactDOM.unmountComponentAtNode(reactNode);
-    ReactDOM.render(componentRegistry.markup(component, data, store), reactNode);
+    ReactDOM.render(componentRegistry.markup(component, { data: data, store: store, wrapWithIntl: false }), reactNode);


Line 10 exceeds the maximum line length of 100 max-len

houndci-bot · 2017-10-12T12:27:54Z

webpack/assets/javascripts/bundle.js

@@ -10,6 +10,9 @@ require('./bundle_multiselect');
 require('./bundle_select2');
 require('./bundle_datatables');
 import compute from './foreman_compute_resource';
+import componentRegistry from './react_app/components/componentRegistry';
+import { locale } from './react_app/common/i18n'
+


More than 1 blank line not allowed no-multiple-empty-lines

houndci-bot · 2017-10-12T12:27:54Z

webpack/assets/javascripts/bundle.js

@@ -10,6 +10,9 @@ require('./bundle_multiselect');
 require('./bundle_select2');
 require('./bundle_datatables');
 import compute from './foreman_compute_resource';
+import componentRegistry from './react_app/components/componentRegistry';
+import { locale } from './react_app/common/i18n'


Missing semicolon semi

waldenraines

I'm wondering about the date formatting. For instance I see the format Oct 16, 13:27 in several places. I think that we should include the year because in some places (sync plans, for instance) the date may be in the past and it may not be clear which year it is referring to.

See also my other comments about displaying locale specific dates as well.

waldenraines · 2017-10-12T14:53:03Z

webpack/assets/javascripts/react_app/components/common/Date/AbsoluteDate.js

+            month='short'
+            hour='2-digit'
+            minute='2-digit'
+            hour12={false} />


I know that most of the world uses 24hr time but to set this to false is saying that we are okay with actively showing US users a time that they may not readily understand.

Shouldn't this be updated by whatever intl library we are using based on the user's locale?

waldenraines · 2017-10-12T14:56:31Z

webpack/assets/javascripts/react_app/components/common/Date/RelativeDate.js

+        month: 'short',
+        hour: '2-digit',
+        minute: '2-digit',
+        hour12: false


This comment also applies to the AbsoluteDate class above but I would think that all of these should be set based on the user's locale and not hardcoded.

danseethaler · 2017-10-12T16:51:46Z

Aside from the couple of configuration things mentioned I think this is great. I really like the approach of utilizing the new register components to make the helper components available.

tstrachota · 2017-10-13T13:57:24Z

Thanks @waldenraines your notes make sense. I'll remove the hardcoded 24 hour format.

My initial idea was to get as close as possible to the date format that we currently sue in the foreman. But your point about the year is valid, we need more components. I looked at the date and time formats that we use in both projects.

Katello uses:

filter	example for en_US	usage notes
shortDate	9/3/10	used for errata issued/updated dates and subscription start/end dates, used 12x
short	9/3/10 12:05 PM	various other places, used 23x
medium	Sep 3, 2010 12:05:08 PM	various other places, used 10x
longDate	September 3, 2010	used only at one place (content view last published)

Foreman uses:

format	example for en_US	usage notes
long	February 12, 2021 17:13	didn't find a place where it would be used
short	Aug 31, 12:52

My assumptions are:

we need a component for showing just a date (without time)
in most of the cases it's fine to show the date and time without year
we still need a component for showing date, time and year for actions that are less frequent
longDate is used only once and can be replaced by date
it's not crutial to display seconds in the dates

Based on that I propose to add following components/formats:

component	example for en_US
LongDateTime	October 13, 2017, 1:54 PM
ShortDateTime	Oct 13, 1:54 PM
Date	9/3/2010

What do you think? The rails helpers in foreman can probably just mount the new components to avoid nuances between the dates rendered form js and at the server side.

@Rohoover I'd also like to know your opinion on that.

houndci-bot · 2017-10-13T13:59:14Z

webpack/assets/javascripts/react_app/components/common/Date/AbsoluteDate.js

+            month="short"
+            hour="2-digit"
+            minute="2-digit" />
+        </span>


Parsing error: Adjacent JSX elements must be wrapped in an enclosing tag

Fixed, I pushed wrong commit by mistake.

waldenraines · 2017-10-13T17:55:09Z

What do you think?

@tstrachota that seems like a good analysis and makes sense to me 👍

Rohoover · 2017-10-13T18:51:50Z

@tstrachota

Looks good.

Just as a note, a lot of users I've talked to would like to be able to set a preference. Not something for this PR, but worth mentioning.

ares · 2017-10-13T20:08:57Z

@tstrachota all above makes sense to me, just one thing

it's not crutial to display seconds in the dates

I think there are some use cases, perhaps at job invocation, seconds matter

waldenraines · 2017-10-26T13:50:39Z

@tstrachota updates on this? I think we have reached a decision on date formatting so we should be good to proceed.

tstrachota · 2017-10-30T19:38:01Z

Updated:

added more components according to the discussion earlier in this PR
components reflect user's timezone
rails helpers now use the components
small refactoring of component registry to provide a single entry point for HOC wrappers

mmoll · 2017-10-30T20:37:01Z

this needs a rebase :)

ohadlevy

thanks, a few minor comments inline from a quick review.

How do we extract the strings from react-int? in case there are new strings/languages that we support?

ohadlevy · 2017-10-31T12:14:06Z

app/helpers/application_helper.rb

@@ -51,23 +51,17 @@ def multiple_with_filter?
  # it supports two formats :short and :long
  # example of long is February 12, 2021 17:13
  # example of short is Aug 31, 12:52
-  def date_time_absolute(time, format = :short)


can you please extract all of the date/time methods to dedicated helper?

I can do that but I'm not sure if some plugins aren't using it already. How about foreman-openscap. Is it used there @ares ?

It's already used, which should not prevent the extraction. I don't understand why it should be in separate helper though? This is application wide helper method. If it was extracted to separate module, I'd still like to include it everywhere.

ohadlevy · 2017-10-31T12:14:49Z

app/helpers/application_helper.rb

+  end
+
+  def mount_date_component(component, time, seconds)
+    date_id = generate_date_id


can't we use something like a timestamp instead of class vars?

ohadlevy · 2017-10-31T12:16:58Z

webpack/assets/javascripts/react_app/common/i18n.js

+import React from 'react';
+import {injectIntl, IntlProvider, addLocaleData} from 'react-intl';
+
+let langAttr = document.getElementsByTagName('html')[0].getAttribute('lang') || 'en';


should be const?

ohadlevy · 2017-10-31T12:19:57Z

webpack/assets/javascripts/react_app/components/common/Date/Date.js

+import { FormattedDate } from 'react-intl';
+import { timezone } from '../../../common/i18n';
+
+// 9/3/2010


Not really. I planned to keep the date format with each component just to give people better clue about what is the output. But I agree it deserves some more explanation.

once you add jest snapshots, we could see the output ;)

ohadlevy · 2017-10-31T12:23:28Z

webpack/assets/javascripts/react_app/components/common/Date/Date.js

+    let date, title;
+
+    if (this.props.data.date) {
+      date = this.props.data.date;


please use const date instead of the above let

ohadlevy · 2017-10-31T12:37:56Z

also, rebase and tests for new components are missings :)

sharvit · 2017-10-31T14:42:02Z

webpack/assets/javascripts/react_app/components/common/Date/LongDateTime.js

+    if (this.props.data.date) {
+      date = this.props.data.date;
+      title = this.props.intl.formatRelative(date);
+      seconds = this.props.data.seconds ? '2-digit' : undefined;


These variables should be defined inside the if block as consts

sharvit · 2017-10-31T14:42:38Z

webpack/assets/javascripts/react_app/components/common/Date/RelativeDateTime.js

+        year: 'numeric',
+        timeZone: timezone
+      });
+


These variables should be defined inside the if block as consts

sharvit · 2017-10-31T14:43:21Z

webpack/assets/javascripts/react_app/components/common/Date/ShortDateTime.js

+      date = this.props.data.date;
+      title = this.props.intl.formatRelative(date);
+      seconds = this.props.data.seconds ? '2-digit' : undefined;
+


These variables should be defined inside the if block as consts

sharvit · 2017-10-31T14:45:06Z

webpack/assets/javascripts/react_app/components/componentRegistry.js

+import { withIntlProvider } from '../common/i18n';
+import { Provider } from 'react-redux';
+
+function getDisplayName(Component) {


This function is also used in common/i18n.js can we reuse it?

sharvit · 2017-10-31T14:47:23Z

webpack/assets/javascripts/react_app/components/componentRegistry.js

+  };
+}
+
+class WrapperFactory {


I believe that the WrapperFactory together with the functions above should be separate into a different file

tstrachota · 2017-11-02T11:15:58Z

Updated according to comments and tests added.

ohadlevy · 2017-11-07T09:50:38Z

@tstrachota can you rebase please? also note the test failures.

ohadlevy · 2017-11-07T09:52:01Z

@ares any feedback on this PR?

ares · 2017-11-07T14:00:36Z

👍 from me, I'm happy the method usage didn't change much, @tstrachota would you mind adding the third param to how to write a plugin wiki after this is merged?

tstrachota · 2017-11-08T14:50:56Z

Rebeased. I need to investigate what went wrong with the tests.

@ares sure, I can update the docs.

houndci-bot · 2017-11-08T15:48:43Z

webpack/assets/javascripts/bundle.js

@@ -25,5 +27,10 @@ window.tfm = Object.assign(window.tfm || {}, {
  numFields: require('./jquery.ui.custom_spinners'),
  reactMounter: require('./react_app/common/MountingService'),
  editor: require('./foreman_editor'),
-  nav: require('./foreman_navigation')
+  nav: require('./foreman_navigation').
+  componentRegistry: componentRegistry,


Parsing error: Unexpected token, expected ,

ohadlevy · 2017-11-08T21:30:57Z

What about i18n? On Nov 8, 2017 5:48 PM, "Hound" <notifications@github.com> wrote: *@houndci-bot* commented on this pull request. ------------------------------ In webpack/assets/javascripts/bundle.js <#4909 (comment)>:

@@ -25,5 +27,10 @@ window.tfm = Object.assign(window.tfm || {}, {

numFields: require('./jquery.ui.custom_spinners'), reactMounter: require('./react_app/common/MountingService'), editor: require('./foreman_editor'), - nav: require('./foreman_navigation') + nav: require('./foreman_navigation'). + componentRegistry: componentRegistry, Parsing error: Unexpected token, expected , — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4909 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOx4So1sA09Q-AMFoPm12pJtkyi0Aeks5s0c1bgaJpZM4P27Ua> .

tstrachota · 2017-11-09T14:11:40Z

Ok, tests are green now.
The previous issue was a typo while resolving rebase conflicts, @ohadlevy . I'm sorry.

waldenraines

This LGTM and worked where tested (also tested with katello) but will allow someone else more familiar with foreman the final approval.

ares · 2017-11-22T14:27:12Z

This needs a rebase

houndci-bot · 2017-12-06T09:54:25Z

webpack/assets/javascripts/react_app/components/wrapperFactory.test.js

+describe('WrapperFactory', () => {
+  it('builds a wrapper', () => {
+    wrapperRegistry.register('name_wrapper', (name) => {
+      return (component) => {


Unexpected block statement surrounding arrow body arrow-body-style

houndci-bot · 2017-12-06T09:54:26Z

webpack/assets/javascripts/react_app/components/wrapperFactory.test.js

+
+describe('WrapperFactory', () => {
+  it('builds a wrapper', () => {
+    wrapperRegistry.register('name_wrapper', (name) => {


Unexpected block statement surrounding arrow body arrow-body-style

houndci-bot · 2017-12-06T09:54:26Z

webpack/assets/javascripts/react_app/components/wrapperFactory.test.js

@@ -0,0 +1,36 @@
+jest.unmock('./wrapperFactory');
+import { WrapperFactory, wrapperRegistry } from './wrapperFactory';


Import in body of module; reorder to top import/first

houndci-bot · 2017-12-06T09:54:26Z

webpack/assets/javascripts/react_app/components/wrapperFactory.js

+  with(name, ...params) {
+    const previousWrapper = this.wrapper;
+
+    this.wrapper = function (component) {


Unexpected unnamed function func-names

houndci-bot · 2017-12-06T09:54:26Z

webpack/assets/javascripts/react_app/components/wrapperFactory.js

+};
+
+export class WrapperFactory {
+  wrapper(component) {


Expected 'this' to be used by class method 'wrapper' class-methods-use-this

houndci-bot · 2017-12-06T09:54:26Z

webpack/assets/javascripts/react_app/components/wrapperFactory.js

@@ -0,0 +1,89 @@
+import React from 'react';
+import { intlProviderWrapper } from '../common/i18n';
+import { Provider } from 'react-redux';


Absolute imports should come before relative imports import/first

houndci-bot · 2017-12-06T09:54:26Z

webpack/assets/javascripts/react_app/components/compontentRegistry.test.js

+      expect(componentRegistry.defaultWrapper).toBeCalledWith(
+        componentRegistry.getComponent(name),
+        'DATA',
+        'STORE'


Missing trailing comma comma-dangle

houndci-bot · 2017-12-06T09:54:26Z

webpack/assets/javascripts/react_app/components/compontentRegistry.test.js

+
+      componentRegistry.markup(name, {
+        data: 'DATA',
+        store: 'STORE'


Missing trailing comma comma-dangle

houndci-bot · 2017-12-06T09:54:27Z

webpack/assets/javascripts/react_app/components/compontentRegistry.test.js

+
+      componentRegistry.register({ name, type: FakeComponent, store: true, data: true });
+      componentRegistry.defaultWrapper = jest.fn((component, data, store) => {
+        return function (cmp) { return cmp; };


Unexpected unnamed function func-names

houndci-bot · 2017-12-06T09:54:27Z

webpack/assets/javascripts/react_app/components/compontentRegistry.test.js

+      const name = 'WrappedMarkupComponent';
+
+      componentRegistry.register({ name, type: FakeComponent, store: true, data: true });
+      componentRegistry.defaultWrapper = jest.fn((component, data, store) => {


Unexpected block statement surrounding arrow body arrow-body-style

tstrachota · 2017-12-07T10:38:35Z

Rebased

waldenraines · 2017-12-07T16:50:08Z

@ares @ohadlevy any other comments here?

ares · 2017-12-07T21:21:10Z

I'm trying to test but I can't install dependencies, I always get this, anyone knows?

npm WARN react-addons-test-utils@15.6.2 requires a peer of react-dom@^15.4.2 but none is installed. You must install peer dependencies yourself.

npm ERR! Cannot read property '0' of undefined

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/ares/.npm/_logs/2017-12-07T21_18_53_635Z-debug.log

EDIT: nvm, looks unrelated, can reproduce now even after resetting back to develop, suggestions welcome though :-)

waldenraines · 2017-12-08T13:54:55Z

EDIT: nvm, looks unrelated, can reproduce now even after resetting back to develop, suggestions welcome though :-)

There's always the big hammer (rm -fr node_modules; npm i). Sometimes npm gets confused it seems.

ohadlevy · 2017-12-11T08:55:51Z

@ares were you able to review this pr?

ares · 2017-12-12T13:12:34Z

Today, npm was in a good mood so I was able to test. All works well, including timezone and locale. If there are no more comments, I'm happy to merge.

ares · 2017-12-13T06:33:34Z

Turns out this adds 15MB of strings because of intl dependency, @tstrachota will take a look whether we can keep intl out of this, if not, we'll revert this commit before 1.17 branching. We should have decision by the end of the day. @xprazak2 FYI

theforeman-bot added Needs testing Not yet reviewed labels Oct 12, 2017

houndci-bot reviewed Oct 12, 2017

View reviewed changes

tstrachota mentioned this pull request Oct 12, 2017

Fixes #21045 - React components for formatting dates Katello/katello#7001

Merged

waldenraines suggested changes Oct 12, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Oct 12, 2017

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Oct 13, 2017

houndci-bot reviewed Oct 13, 2017

View reviewed changes

ohadlevy reviewed Oct 31, 2017

View reviewed changes

sharvit reviewed Oct 31, 2017

View reviewed changes

sharvit suggested changes Oct 31, 2017

View reviewed changes

theforeman-bot added Waiting on contributor Needs re-review Needs testing and removed Needs re-review Waiting on contributor labels Oct 31, 2017

sharvit approved these changes Nov 2, 2017

View reviewed changes

houndci-bot reviewed Nov 8, 2017

View reviewed changes

waldenraines approved these changes Nov 9, 2017

View reviewed changes

ares added Waiting on contributor and removed Needs re-review Needs testing labels Nov 22, 2017

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Dec 6, 2017

houndci-bot reviewed Dec 6, 2017

View reviewed changes

Fixes #21312 - components for formatting dates

89b8e42

ares approved these changes Dec 12, 2017

View reviewed changes

ohadlevy merged commit f2121ff into theforeman:develop Dec 12, 2017

dLobatog mentioned this pull request Dec 13, 2017

Deploy dynflowd [deb] theforeman/foreman-packaging#1959

Merged

xprazak2 mentioned this pull request Dec 14, 2017

Add intl package theforeman/foreman-packaging#1981

Merged

4 tasks

		@@ -0,0 +1,36 @@
		jest.unmock('./wrapperFactory');
		import { WrapperFactory, wrapperRegistry } from './wrapperFactory';

Fixes #21312 - components for formatting dates #4909

Fixes #21312 - components for formatting dates #4909

Conversation

tstrachota commented Oct 12, 2017 • edited

theforeman-bot commented Oct 12, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waldenraines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danseethaler commented Oct 12, 2017

tstrachota commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waldenraines commented Oct 13, 2017

Rohoover commented Oct 13, 2017

ares commented Oct 13, 2017

waldenraines commented Oct 26, 2017

tstrachota commented Oct 30, 2017

mmoll commented Oct 30, 2017

ohadlevy left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohadlevy commented Oct 31, 2017

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

tstrachota commented Nov 2, 2017

ohadlevy commented Nov 7, 2017

ohadlevy commented Nov 7, 2017

ares commented Nov 7, 2017

tstrachota commented Nov 8, 2017

Choose a reason for hiding this comment

ohadlevy commented Nov 8, 2017 via email

tstrachota commented Nov 9, 2017

waldenraines left a comment

Choose a reason for hiding this comment

ares commented Nov 22, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstrachota commented Dec 7, 2017

waldenraines commented Dec 7, 2017

ares commented Dec 7, 2017 • edited

waldenraines commented Dec 8, 2017

ohadlevy commented Dec 11, 2017

ares commented Dec 12, 2017

ares commented Dec 13, 2017

tstrachota commented Oct 12, 2017 •

edited

ares commented Dec 7, 2017 •

edited