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

[#157929923] add-badge-messages #979

Merged
merged 20 commits into from
May 20, 2019
Merged

Conversation

matgentili
Copy link
Contributor

[#157929923] not completed. For the moment the badge is visible in all the tabs. Logic for badge is not implemented

[#157929923] not completed. For the moment the badge is visible in all the tabs. Logic for badge is not implemented
@digitalcitizenship
Copy link

digitalcitizenship commented Apr 24, 2019

Warnings
⚠️

Remember to fix the PR title by removing WIP wording when ready

Affected stories

  • 🌟 #157929923: Implementare badge nel tab dei messaggi con il numero di messaggi non letti

Generated by 🚫 dangerJS

@fpersico
Copy link
Contributor

@matgentili @cloudify We are using native-base as UI library and it also has a Badge component. I don't think we need to add another UI library like react-native-elements.

@matgentili
Copy link
Contributor Author

@fpersico ok, i remove react-native-elements library and use native-base.

@@ -71,7 +71,6 @@ class MessageListComponent extends React.Component<Props> {
const refreshControl = (
<RefreshControl refreshing={refreshing} onRefresh={onRefresh} />
);

Copy link
Contributor

Choose a reason for hiding this comment

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

why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, i think i removed the line during implementing an other const that i removed later

@@ -1,12 +1,6 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

why you removed the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I didn't even notice it. Most likely I removed it while importing a library

Copy link
Contributor

Choose a reason for hiding this comment

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

Still removed

[#157929923] not completed. Badge graphic is ok on Android, I must check it on iOS. The logic for the badge counter has not implemented yet.
@codecov
Copy link

codecov bot commented Apr 24, 2019

Codecov Report

Merging #979 into master will decrease coverage by 0.2%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #979      +/-   ##
==========================================
- Coverage    46.5%   46.29%   -0.21%     
==========================================
  Files         201      199       -2     
  Lines        4455     4378      -77     
  Branches      873      818      -55     
==========================================
- Hits         2072     2027      -45     
+ Misses       2376     2346      -30     
+ Partials        7        5       -2

@codecov
Copy link

codecov bot commented Apr 24, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@95ad2c4). Click here to learn what that means.
The diff coverage is 40.62%.

@@            Coverage Diff            @@
##             master     #979   +/-   ##
=========================================
  Coverage          ?   45.21%           
=========================================
  Files             ?      210           
  Lines             ?     4837           
  Branches          ?      948           
=========================================
  Hits              ?     2187           
  Misses            ?     2643           
  Partials          ?        7

[#157929923] not completed. Badge graphic is ok on Android, I must check it on iOS. The logic for the badge counter has not implemented yet.
package.json Outdated
@@ -99,6 +99,7 @@
"@types/react": "16.8.10",
"@types/react-native": "0.57.42",
"@types/react-native-background-timer": "^2.0.0",
"@types/react-native-elements": "^0.18.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

this need to be removed too

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

@@ -12,7 +12,8 @@ import { MessageState } from "../../store/reducers/entities/messages/messagesByI
import { PaymentByRptIdState } from "../../store/reducers/entities/payments";
import { ServicesByIdState } from "../../store/reducers/entities/services/servicesById";
import { MessageListItemComponent } from "./MessageListItemComponent";

// tslint:disable-next-line: no-var-keyword
var messagesToRead = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

don't use vars, the tslint rule is there for a reason :)

Copy link
Contributor Author

@matgentili matgentili Apr 29, 2019

Choose a reason for hiding this comment

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

Ok, can i use let?

const refreshControl = (
<RefreshControl refreshing={refreshing} onRefresh={onRefresh} />
);

messagesToRead = messages.filter(obj => !obj.isRead).length;
alert(messagesToRead);
Copy link
Contributor

Choose a reason for hiding this comment

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

what's this alert?

Copy link
Contributor Author

@matgentili matgentili Apr 29, 2019

Choose a reason for hiding this comment

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

It was only for tests and I forgot to remove it

yarn.lock Outdated
@@ -1071,6 +1071,13 @@
version "2.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

since you didn't add any package, yarn.lock must not change

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

@matgentili please check this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fpersico Can i remove yarn.lock ? I don't add any package

Copy link
Contributor

Choose a reason for hiding this comment

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

Just revert the changes.

[#157929923] add-badge-messages. Modified badge layout and added method for reading the number of messages. Tested on ios and android
@matgentili matgentili marked this pull request as ready for review April 29, 2019 15:06
@matgentili matgentili requested a review from cloudify April 29, 2019 15:09
@@ -178,20 +180,27 @@ const navigation = createBottomTabNavigator(
tabBarIcon: (options: { tintColor: string | null; focused: boolean }) => {
const { routeName } = nav.state;
const iconName: string = getIcon(routeName);
const messagesToRead = MessageListComponent.prototype.getMessagesToRead();
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain how this works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I take from the component that manages the list of messages the number of unread messages and save them in a constant which is then used to update the view. Do you think that could be fine?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it could possibly work, did you tested it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have tested it on ios simulator and on android device and when I open a message the badge number is updated correctly.

@cloudify
Copy link
Contributor

@matgentili you should create a custom component that subscribes to the redux store to get the list of messages

@cloudify
Copy link
Contributor

cloudify commented May 3, 2019

@matgentili any update here?

@matgentili
Copy link
Contributor Author

@cloudify I used Redux store as you suggested, now i'm testing it and soon I commit all ;)

[#157929923]-add-badge-message-tab. Added store redux for badge
@fpersico fpersico changed the title [#157929923] add-badge-messages [#157929923] [WIP] add-badge-messages May 7, 2019
matgentili and others added 2 commits May 13, 2019 14:15
Completed badge implementation. It works correctly on ios and android.
@matgentili
Copy link
Contributor Author

badge_message

fix - Now the badge number is retrieved using BadgeSelector instead of passing it during navigation
@matgentili matgentili requested a review from cloudify May 13, 2019 13:14
matgentili and others added 3 commits May 15, 2019 14:38
Fix implementation badge messages. Now I don't use reducer but i get the number for badge by list of messages
Added control if badge number is greater than 99
{messagesUnread > 0 ? (
Platform.OS === "ios" ? (
<Badge style={styles.badgeStyle}>
<Text style={[styles.textBadgeStyle, { top: 0 }]}>
Copy link
Contributor

Choose a reason for hiding this comment

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

No inline style

/**
* Filters the list of messages and returns the number of unread messages.
*/
const getNumberMessagesUnread = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Convert this to a redux selector


const mapStateToProps = (state: GlobalState) => ({
messagesUnread:
getNumberMessagesUnread(lexicallyOrderedMessagesStateSelector(state)) < 99
Copy link
Contributor

Choose a reason for hiding this comment

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

You are calling the function twice!

@@ -1,12 +1,6 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Still removed

@@ -87,6 +79,30 @@ const styles = StyleSheet.create({
shadowRadius: variables.footerShadowRadius,
// Android shadow
elevation: variables.footerElevation
},
textBadgeStyle: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not used anymore?

@fpersico
Copy link
Contributor

@matgentili Please clean this PR because it contains unused stuff.

matgentili and others added 2 commits May 20, 2019 11:15
@matgentili matgentili requested a review from fpersico May 20, 2019 10:27
yarn.lock Outdated
@@ -1071,6 +1071,13 @@
version "2.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

@matgentili please check this.

@matgentili matgentili requested a review from fpersico May 20, 2019 12:30
@matgentili matgentili changed the title [#157929923] [WIP] add-badge-messages [#157929923] add-badge-messages May 20, 2019
@fpersico fpersico merged commit b04228a into master May 20, 2019
matgentili added a commit that referenced this pull request May 22, 2019
Now the badge's text is centered. I moved one pixel.
To be added to [#157929923] add-badge-messages #979
fpersico pushed a commit that referenced this pull request May 23, 2019
Now the badge's text is centered. I moved one pixel.
To be added to [#157929923] add-badge-messages #979
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.

5 participants