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

Add market price on feed for every crypto #1189

Merged
merged 3 commits into from
Dec 13, 2017

Conversation

jm90m
Copy link
Contributor

@jm90m jm90m commented Dec 12, 2017

Fixes #1170

Changes:

  • Add FeedSidebar component & CryptoTrendingCharts component
  • add cryptosHelper to search for cryptos within the top 100 crypto constants

screen shot 2017-12-12 at 3 29 34 pm
screen shot 2017-12-12 at 3 29 49 pm

* Fixes #1170
* Changes:
	- Add FeedSidebar component & CryptoTrendingCharts component
	- add cryptosHelper to search for cryptos within the top 100 crypto constants
@jm90m jm90m added the feature label Dec 12, 2017
@jm90m jm90m added this to the v2.2 milestone Dec 12, 2017
@jm90m jm90m self-assigned this Dec 12, 2017
@bonustrack bonustrack temporarily deployed to busy-master-pr-1189 December 12, 2017 20:30 Inactive
@bonustrack
Copy link
Contributor

@jm90m
Copy link
Contributor Author

jm90m commented Dec 12, 2017

@bonustrack yup just updated it, please check again, thx

@bonustrack bonustrack temporarily deployed to busy-master-pr-1189 December 12, 2017 21:04 Inactive
`https://min-api.cryptocompare.com/data/histoday?fsym=${symbol}&tsym=USD&limit=7`,
).then(res => res.json());

class CryptoTrendingCharts extends React.Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should avoid having duplicate code, i think this component should also be used on the Steem and SBD market block on wallet pages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Steem & SBD market blocks are both combined into one, thats why I kept the SteemTrendingCharts component...

I can make it look like this if you want?

screen shot 2017-12-12 at 4 18 10 pm

and it's not really duplicate code, since if you look at steem trending charts component, there is a few different logic in there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you just make that the component can take an array of symbol instead of only one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I can but this would only be for one case? just for this specific scenario? Is that worth doing, when we are going to update the charts design anyways?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are not doing this we are going to update the charts in 2 different places isn't 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.

yeah thats true, I'll see what I can come up with .

@jm90m
Copy link
Contributor Author

jm90m commented Dec 13, 2017

@bonustrack just updated it so their is now a CryptoChart common component that is used. so less duplicate code now.

@jm90m jm90m merged commit 32a4c89 into master Dec 13, 2017
@jm90m jm90m deleted the jm-add-chart-side-bar-for-crypto branch December 13, 2017 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants