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.

Sign up for GitHub

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

Jump to bottom

Issue 315 - Add Chart Clamp To Settings Dropdown #373

Merged
merged 2 commits into from
Sep 14, 2017
Merged

Issue 315 - Add Chart Clamp To Settings Dropdown #373

merged 2 commits into from
Sep 14, 2017

Conversation

fletchertyler914
Copy link
Contributor

#315 Fix. Add Toggle In Settings Dropdown.

@wmbutler wmbutler added the [3] Feature Classification indicating the addition of novel functionality to the design label Sep 10, 2017
@wmbutler wmbutler added this to the 170914 milestone Sep 10, 2017
svk31
svk31 suggested changes Sep 12, 2017
View reviewed changes
Copy link
Contributor

@svk31 svk31 left a comment

Choose a reason for hiding this comment

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

For some reason this causes a full reset of the price chart whenever you click on one of the dropdowns for settings or indicators. To reproduce, just zoom or pan the chart, then click the cog. I found it quite frustrating so please look into it to see if it can be fixed.

web/app/components/Exchange/PriceChartD3.jsx Outdated
@@ -36,6 +36,7 @@ class CandleStickChartWithZoomPan extends React.Component {
this.state = {
enableTrendLine: false,
enableFib: false,
clampEnabled: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable isn't used anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed and recommitted.

@fletchertyler914
Copy link
Contributor Author

@svk31 the chart behavior you described exists already due to the way its rendering the chart on update. You can see this by checking out the bitshares/master branch without this update and repeating the steps you mentioned. Since this is outside of the scope of this feature, we can create a new bug to address the fix. I don't like it either, but I'm not sure there is a way around this with the current library.

@svk31
Copy link
Contributor

svk31 commented Sep 13, 2017

No I'm afraid this behaviour does not exist in the current version in staging without these changes.

@wmbutler
Copy link
Contributor

@fletchertyler914 Should I push this to next Sprint? We are releasing tomorrow.

@fletchertyler914
Copy link
Contributor Author

fletchertyler914 commented Sep 13, 2017 •
edited
Loading

@svk31 I just pulled the current staging branch and it is doing this same behavior for me. If i zoom in on the chart and click the settings cog it will reset the zoom. Actually I just checked master, bitshares, and staging and they are all doing the same thing for me. Is this not happening for anyone else? @wmbutler?

Here's a screencast

@fletchertyler914 fletchertyler914 changed the title Add Chart Clamp To Settings Dropdown Issue 315 - Add Chart Clamp To Settings Dropdown Sep 13, 2017
@fletchertyler914
Copy link
Contributor Author

@svk31 I just cloned this repo, ran the master branch, and the behavior still exist. If it matters, I am running Chrome v60 on a Windows 10 machine. It's definitely an issue that was already there since this wasn't running my fork.

@wmbutler
Copy link
Contributor

@svk31 This is a really weird disconnect here. Did you see tyler's screencast?

@svk31
Copy link
Contributor

svk31 commented Sep 14, 2017

Ok I see what's happening, I have made a local modification of my react-stockcharts files as shown in this PR rrag/react-stockcharts#332. That fixes the issue in the current version, but it reappears when the clamp is introduced.

@svk31 svk31 merged commit 4b5e84c into bitshares:bitshares Sep 14, 2017
svk31 pushed a commit that referenced this pull request Sep 14, 2017
@fletchertyler914 @svk31
Issue 315 - Add Chart Clamp To Settings Dropdown (#373)
e965de0 
* Add Chart Clamp To Settings Dropdown

* Removed Unused Variable

Merging as is and then we'll see about handling the redraw bug in the next sprint.
svk31 pushed a commit that referenced this pull request Oct 1, 2017
@svk31
#465 remove transwiser reference (#475)
c6055ed 
* Update README.md

* Issue 315 - Add Chart Clamp To Settings Dropdown (#373)

* Add Chart Clamp To Settings Dropdown

* Removed Unused Variable

Merging as is and then we'll see about handling the redraw bug in the next sprint.

* remove Transwiser reference per #465
svk31 added a commit that referenced this pull request Oct 13, 2017
@svk31
Fix issue #294 (#575)
7b96dfd 
* Update README.md

* Issue 315 - Add Chart Clamp To Settings Dropdown (#373)

* Add Chart Clamp To Settings Dropdown

* Removed Unused Variable

Merging as is and then we'll see about handling the redraw bug in the next sprint.

* More intuitive vesting stats in UI - issue #294

* pending_vested_fees->pending_immediate_fees

* Fees terminology

I took a stab at this.

* Remove fees_pending and fees_vested text, update fee texts #294
svk31 pushed a commit that referenced this pull request Oct 17, 2017
@svk31
#358b (#597)
759e474 
* Update README.md

* Issue 315 - Add Chart Clamp To Settings Dropdown (#373)

* Add Chart Clamp To Settings Dropdown

* Removed Unused Variable

Merging as is and then we'll see about handling the redraw bug in the next sprint.

* Fix #358b lowercase username <li>

* Header.jsx lowercase accountnames 

Remove vertical-align per comment
svk31 pushed a commit that referenced this pull request Oct 20, 2017
@svk31
#590 (#617)
8fb2902 
* Update README.md

* Issue 315 - Add Chart Clamp To Settings Dropdown (#373)

* Add Chart Clamp To Settings Dropdown

* Removed Unused Variable

Merging as is and then we'll see about handling the redraw bug in the next sprint.

* resolve conflict

* Update README.md

* adopt 'Burn asset' nomenclature #590

* update README
svk31 pushed a commit that referenced this pull request Oct 23, 2017
@svk31
Fix #580 (#626)
2456823 
* Update README.md

* Issue 315 - Add Chart Clamp To Settings Dropdown (#373)

* Add Chart Clamp To Settings Dropdown

* Removed Unused Variable

Merging as is and then we'll see about handling the redraw bug in the next sprint.

* resolve conflict

* Update README.md

* Fix #580 add linethrough class

* Fix #580 Annual fee
svk31 pushed a commit that referenced this pull request Oct 23, 2017
@svk31
622 Harmonized login (#629)
e0b43d8 
* Update README.md

* Issue 315 - Add Chart Clamp To Settings Dropdown (#373)

* Add Chart Clamp To Settings Dropdown

* Removed Unused Variable

Merging as is and then we'll see about handling the redraw bug in the next sprint.

* resolve conflict

* Update README.md

* Local Wallet Login
svk31 pushed a commit that referenced this pull request Oct 25, 2017
@svk31
Fix #455 Remove trollbox (#602)
6594f6d 
* Update README.md

* Issue 315 - Add Chart Clamp To Settings Dropdown (#373)

* Add Chart Clamp To Settings Dropdown

* Removed Unused Variable

Merging as is and then we'll see about handling the redraw bug in the next sprint.

* resolve conflict

* Update README.md

* #455:remove trollbox entirely
svk31 pushed a commit that referenced this pull request Oct 31, 2017
@svk31
Clean up README (#666)
811b9cc 
* Update README.md

* Issue 315 - Add Chart Clamp To Settings Dropdown (#373)

* Add Chart Clamp To Settings Dropdown

* Removed Unused Variable

Merging as is and then we'll see about handling the redraw bug in the next sprint.

* resolve conflict

* Update README.md

* Update README.md

* clean up readme
CryptoBridge pushed a commit to CryptoBridge/cryptobridge-ui that referenced this pull request Nov 21, 2017
@happyconcepts @svk31
Clean up README (bitshares#666)
0c55e54 
* Update README.md

* Issue 315 - Add Chart Clamp To Settings Dropdown (bitshares#373)

* Add Chart Clamp To Settings Dropdown

* Removed Unused Variable

Merging as is and then we'll see about handling the redraw bug in the next sprint.

* resolve conflict

* Update README.md

* Update README.md

* clean up readme
svk31 pushed a commit that referenced this pull request Jan 2, 2018
@svk31
Remove Transwiser from CSP (#940)
c015c2d 
* Update README.md

* Issue 315 - Add Chart Clamp To Settings Dropdown (#373)

* Add Chart Clamp To Settings Dropdown

* Removed Unused Variable

Merging as is and then we'll see about handling the redraw bug in the next sprint.

* Remove transwiser.com from Content Security Policy
Cryptolero pushed a commit to CryptoBridge/cryptobridge-ui that referenced this pull request Nov 2, 2018
@happyconcepts @svk31
Remove Transwiser from CSP (bitshares#940)
07dcb5c 
* Update README.md

* Issue 315 - Add Chart Clamp To Settings Dropdown (bitshares#373)

* Add Chart Clamp To Settings Dropdown

* Removed Unused Variable

Merging as is and then we'll see about handling the redraw bug in the next sprint.

* Remove transwiser.com from Content Security Policy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svk31 svk31 svk31 requested changes

Assignees
No one assigned
Labels
[3] Feature Classification indicating the addition of novel functionality to the design
Projects
None yet
Milestone
170914
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fletchertyler914 @svk31 @wmbutler