[#163591901,#163591918,#163591935] Messages Inbox/Deadlines/Archive Tabs (v2) #823

fpersico · 2019-02-14T09:08:01Z

A replacement for #821 with only changes strictly needed and better architecture.

digitalcitizenship · 2019-02-14T09:09:33Z

Affected stories

🌟 #163591901: Come CIT voglio avere delle TAB che filtrano i messaggi
🌟 #163591918: Come CIT voglio poter archiviare i messaggi
🌟 #163591935: Come CIT voglio poter vedere filtrare, fra i messaggi, solo quelli che hanno una scadenza

Generated by 🚫 dangerJS

codecov · 2019-02-14T09:10:14Z

Codecov Report

Merging #823 into master will decrease coverage by 0.57%.
The diff coverage is 37.32%.

@@            Coverage Diff             @@
##           master     #823      +/-   ##
==========================================
- Coverage   46.69%   46.11%   -0.58%     
==========================================
  Files         178      187       +9     
  Lines        3602     3801     +199     
  Branches      703      727      +24     
==========================================
+ Hits         1682     1753      +71     
- Misses       1918     2046     +128     
  Partials        2        2

cloudify · 2019-02-14T14:23:44Z

ts/components/helpers/withMessagesSelection.tsx

+        selectedMessageIds,
+        toggleMessageSelection: this.toggleMessageSelection,
+        resetSelection: this.resetSelection
+      } as P;


is the cast really needed?

Sadly yes :(

any idea why?

From my research it is related to microsoft/TypeScript#28938

have you tried this option - it looks safer than casting the merged props

Yes i remeber that i also tested that solution and had no problem. Going to change.

cloudify · 2019-02-14T14:25:27Z

ts/components/helpers/withMessagesSelection.tsx

+
+type State = {
+  isSelectionModeEnabled: boolean;
+  selectedMessageIds: Map<string, true>;


you should probably use an Option<Set<string>> here

cloudify · 2019-02-14T14:27:51Z

ts/components/helpers/withMessagesSelection.tsx

+
+    private resetSelection = () => {
+      this.setState({
+        isSelectionModeEnabled: false,


if you use an Option<Set<string>> you can just set selectedMessageIds to none to encode both the fact that selection mode is off and no messages are selected

cloudify · 2019-02-14T14:28:03Z

ts/components/helpers/withMessagesSelection.tsx

+        selectedMessageIds.get(id)
+          ? selectedMessageIds.delete(id)
+          : selectedMessageIds.set(id, true);
+        return { isSelectionModeEnabled: true, selectedMessageIds };


see next comment

cloudify · 2019-02-14T14:29:39Z

ts/components/helpers/withMessagesSelection.tsx

+      return <WrappedComponent {...mergedProps} />;
+    }
+
+    private toggleMessageSelection = (id: string) => {


comment this function

cloudify · 2019-02-14T14:52:17Z

ts/store/reducers/entities/messages/index.ts

      ids.map(messageId => messageStateById[messageId]).filter(isDefined)
-    )
+    ),
+    lastUpdate: Date.now()


I'm not sure why we need this, shouldn't createSelector memoize the result making it possible ti swallow compare it with the previous value?

check out https://github.com/reduxjs/reselect#createselectorinputselectors--inputselectors-resultfunc + defaultMemoize and createSelectorCreator

cloudify · 2019-02-14T14:56:10Z

ts/components/messages/MessagesArchive.tsx

+
+  private handleOnPressItem = (id: string) => {
+    if (this.props.isSelectionModeEnabled) {
+      this.handleOnLongPressItem(id);


shouldn't this be handleOnPressItem ?

I have to add a comment here. If we are in selection mode a "press" need act like the "longPress" (select the item).

cloudify · 2019-02-14T14:59:04Z

ts/screens/messages/MessagesHomeScreen.tsx

+  private setMessagesArchivedState = (
+    ids: ReadonlyArray<string>,
+    archived: boolean
+  ) => this.props.dispatch(setMessagesArchivedState(ids, archived));


use a map dispatch to props here

cloudify · 2019-02-14T15:01:20Z

ts/store/actions/messages.ts

@@ -44,9 +44,16 @@ export const setMessageReadState = createAction(
  resolve => (id: string, read: boolean) => resolve({ id, read }, { id, read })
 );

+export const setMessagesArchivedState = createAction(
+  "MESSAGES_SET_ARCHIVED",
+  resolve => (ids: ReadonlyArray<string>, archived: boolean) =>


instead of a boolean, I would encode the actual states with and ADT, e.g. :

Suggested change

resolve => (ids: ReadonlyArray<string>, archived: boolean) =>

resolve => (ids: ReadonlyArray<string>, state: "VISIBLE" | "ARCHIVED") =>

much clearer and less error prone than a boolean

I have a diferrent opinion about this for a couple of reason:

"visibile" is not an alternative of "archived". i mean what is visible in a screen can be not visibile in onother. The MessageSearchResult component (coming) display both "visible" and "archived" messages (the same is true for the Deadlines)

a boolean as representation of the archived state is more like the one that in the future will be implemented server side (i mean the message object returned by the api will have a archived boolean property)

if tomorrow we want to add a new state like "starred" seem unreal to have "visible" | "archived" | "starred"

What you think?

cloudify · 2019-02-14T15:13:43Z

ts/types/MessageWithContentAndDueDatePO.ts

+import { MessageContent } from "../../definitions/backend/MessageContent";
+import { MessageWithContentPO } from "./MessageWithContentPO";
+
+type MessageContentWithDueDate = MessageContent & {


you can use a utility method called requireProperty that we used for pagopa - see an example here https://github.com/teamdigitale/italia-app/blob/master/ts/types/pagopa.ts#L52

cloudify · 2019-02-15T16:18:24Z

locales/en/index.yml

@@ -459,6 +459,8 @@ messages:
      fetchCalendars: Error fetching calendars
  unknownSender: Unknown sender
  noContent: Content not available
+  agenda:
+    sectionDate: "dddd D MMMM"


this date format is not specific of the agenda view, it should be under the global namespace

fpersico requested a review from cloudify February 14, 2019 09:08

francescopersico added 5 commits February 14, 2019 10:10

Add Inbox tab

c11a1e7

Extract messagesSelection logic into HOC

c9de91c

Add comments

22200e4

Add additional comments

9b266d9

Revert unnecessary export

c054f16

fpersico force-pushed the messages-inbox-tab branch from 44c436e to c054f16 Compare February 14, 2019 09:11

Add Archive tab

f2fbc7b

fpersico changed the title ~~[#163591901,#163591918] Messages Inbox Tab (v2)~~ [#163591901,#163591918] Messages Inbox/Archive Tab (v2) Feb 14, 2019

Add Deadlines tab

978573a

fpersico changed the title ~~[#163591901,#163591918] Messages Inbox/Archive Tab (v2)~~ [#163591901,#163591918] Messages Inbox/Deadlines/Archive Tabs (v2) Feb 14, 2019

fpersico changed the title ~~[#163591901,#163591918] Messages Inbox/Deadlines/Archive Tabs (v2)~~ [#163591901,#163591918,#163591935] Messages Inbox/Deadlines/Archive Tabs (v2) Feb 14, 2019

cloudify suggested changes Feb 14, 2019

View reviewed changes

francescopersico added 3 commits February 14, 2019 18:40

Use Option instead of two different props

101dcb3

Fixes and improvements

0e253ee

Remove unnecessary lastUpdate

0e0f7ec

cloudify suggested changes Feb 15, 2019

View reviewed changes

Add date format to global namespace

e0ca3e0

cloudify approved these changes Feb 15, 2019

View reviewed changes

cloudify merged commit aca9f05 into master Feb 15, 2019

cloudify deleted the messages-inbox-tab branch February 15, 2019 17:14

fpersico mentioned this pull request Feb 25, 2019

[#163591901,#163591918] Messages Inbox Tab #821

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#163591901,#163591918,#163591935] Messages Inbox/Deadlines/Archive Tabs (v2) #823

[#163591901,#163591918,#163591935] Messages Inbox/Deadlines/Archive Tabs (v2) #823

fpersico commented Feb 14, 2019 •

edited

Loading

digitalcitizenship commented Feb 14, 2019 •

edited

Loading

codecov bot commented Feb 14, 2019 •

edited

Loading

cloudify Feb 14, 2019

fpersico Feb 14, 2019

cloudify Feb 14, 2019

fpersico Feb 14, 2019

cloudify Feb 14, 2019

fpersico Feb 14, 2019

cloudify Feb 14, 2019

cloudify Feb 14, 2019

cloudify Feb 14, 2019

cloudify Feb 14, 2019

cloudify Feb 14, 2019

cloudify Feb 14, 2019

cloudify Feb 14, 2019

fpersico Feb 14, 2019

cloudify Feb 14, 2019

cloudify Feb 14, 2019

fpersico Feb 15, 2019

cloudify Feb 14, 2019

cloudify Feb 15, 2019

	resolve => (ids: ReadonlyArray<string>, archived: boolean) =>
	resolve => (ids: ReadonlyArray<string>, state: "VISIBLE" \| "ARCHIVED") =>

[#163591901,#163591918,#163591935] Messages Inbox/Deadlines/Archive Tabs (v2) #823

[#163591901,#163591918,#163591935] Messages Inbox/Deadlines/Archive Tabs (v2) #823

Conversation

fpersico commented Feb 14, 2019 • edited Loading

digitalcitizenship commented Feb 14, 2019 • edited Loading

Affected stories

codecov bot commented Feb 14, 2019 • edited Loading

Codecov Report

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

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

fpersico commented Feb 14, 2019 •

edited

Loading

digitalcitizenship commented Feb 14, 2019 •

edited

Loading

codecov bot commented Feb 14, 2019 •

edited

Loading