-
Notifications
You must be signed in to change notification settings - Fork 77
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
Updated Zulip.js #180
Updated Zulip.js #180
Conversation
I think the issue is solved. #72
The issue is solved. #72 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comments @msaini0r 👇
@@ -59,10 +59,6 @@ class Zulip extends Component { | |||
<b>Streams: </b> | |||
<br /> | |||
<ul> | |||
<li> | |||
<b>#newcomers: </b> | |||
{zulipstat[0] ? zulipstat[0].newcomers_messages : 0} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you removing this?
You need to change the condition so that it can render without any errors.
{zulipstat && zulipstat[0] && zulipstat[0].newcomers_message ? zulipstat[0].newcomers_message : 0}
Same change for other lines also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codesankalp can you please check if I made the right changes?
src/components/Zulip.js
Outdated
@@ -1,121 +0,0 @@ | |||
import React, { Component } from 'react'; | |||
import { connect } from 'react-redux'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msaini0r You have removed the whole code from this file. Please revert it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@can you give me instructions how to revert it. @codesankalp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codesankalp should i delete that branch?
This reverts commit 9593b3f.
src/components/Zulip.js
Outdated
@@ -61,23 +61,23 @@ class Zulip extends Component { | |||
<ul> | |||
<li> | |||
<b>#newcomers: </b> | |||
{zulipstat[0] ? zulipstat[0].newcomers_messages : 0} | |||
{zulipstat && zulipstat[0] && zulipstat[0].newcomers_message ? zulipstat[0].newcomers_message : 0} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line looks repeated for zulipstat && zulipstat[0]
. Can you please remove this zulipstat && zulipstat[0]
from each line and add zulipstat && zulipstat[0]
in parent tag (ul
)?
{zulipstat && zulipstat[0] && zulipstat[0].newcomers_message ? zulipstat[0].newcomers_message : 0} | |
{zulipstat[0].newcomers_message ? zulipstat[0].newcomers_message : 0} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codesankalp check now changes done.
src/components/Zulip.js
Outdated
@@ -58,26 +58,26 @@ class Zulip extends Component { | |||
<> | |||
<b>Streams: </b> | |||
<br /> | |||
<ul> | |||
<zulipstat && zulipstat[0]> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<zulipstat && zulipstat[0]> | |
{zulipstat && zulipstat[0] ? <ul>below code<ul> : null} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codesankalp see!!
@msaini0r please edit the pull request description with a description of the changes and how you tested this? This is a good practice for future PRs, whether it's in our community or other projects, and good for documentation purposes 🤗 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msaini0r could you please check why the PR check is failing? This seems to be related to your PR. Once you fix the failing check I can approve this PR and merge it.
removed Ready to Merge label as the PR checks are failing cc @Aaishpra |
@isabelcosta I am not able to find the issue. I would like to worrk on some other issues |
Hi @msaini0r only linters and formatters check is failing if you can fix them it would be great
|
@Aaishpra already tried npm run lint but it is showing me an error. |
Description
Fixes #72
Type of Change:
Code/Quality Assurance Only
How Has This Been Tested?
Checklist: