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

Improved Link component #215

Merged
merged 10 commits into from Jul 12, 2018

Conversation

Projects
None yet
4 participants
@valentijnnieman
Copy link
Contributor

commented Jun 22, 2018

This improves the Link component somewhat, so that users can right-click on it etc. It also scrolls back to top when clicking a link. The fix is basically adding a href attribute to the tag rendered, so that the browser treats it as a native link. Should fix #99

@valentijnnieman valentijnnieman requested a review from chriddyp Jun 22, 2018

constructor(props) {
super(props);
this.updateLocation = this.updateLocation.bind(this);
}

This comment has been minimized.

Copy link
@chriddyp

chriddyp Jun 22, 2018

Member

We should really enforce style with eslint, but for now our convention has been tabs with 4 spaces (to match the code that we write in our python context)

This comment has been minimized.

Copy link
@valentijnnieman

valentijnnieman Jun 22, 2018

Author Contributor

Ah yep, I have Prettier running by default on save in VS Code, so if there's nothing in eslint then it will format it like this I guess!

This comment has been minimized.

Copy link
@bpostlethwaite

bpostlethwaite Jun 22, 2018

Member

I'll set up prettier and eslint in CI

This comment has been minimized.

Copy link
@bpostlethwaite

bpostlethwaite Jun 22, 2018

Member

including a prettierrc in the root.

@@ -2,69 +2,72 @@

import PropTypes from 'prop-types';

import React, {Component} from 'react';
import React, { Component } from 'react';

This comment has been minimized.

Copy link
@chriddyp

chriddyp Jun 22, 2018

Member

Similarly here - this isn't enforced in eslint (although it should be), but our convention has been no spaces around {} imports

};
var evt = document.createEvent('CustomEvent');
evt.initCustomEvent(event, params.bubbles, params.cancelable, params.detail);
return evt;

This comment has been minimized.

Copy link
@chriddyp

chriddyp Jun 22, 2018

Member

In general, it'd be helpful if you split out commits that did style updates vs commits that added or removed code. As a reviewer, it takes more time to understand which lines have actual changes

This comment has been minimized.

Copy link
@valentijnnieman

valentijnnieman Jun 22, 2018

Author Contributor

I think this is another case of the Prettier autoformatter doing "it's job" :) But duly noted!

}
updateLocation(e) {
e.preventDefault(); // prevent anchor from updating location
const { href, refresh } = this.props;

This comment has been minimized.

Copy link
@chriddyp

chriddyp Jun 22, 2018

Member

Similarly here, our convention is no white spaces around {}, so const {href, refresh} = this.props

@chriddyp

This comment has been minimized.

Copy link
Member

commented Jun 22, 2018

This is very interesting. I wonder how we can test this with our integration tests.

  • For the scroll behaviour, maybe we can create a multi-page dash app example where the pages are really long and the links are towards the bottom. Ideally, after clicking on a link, we see the content at the top of the second page rather than the bottom of the second page.
  • We also need to test that clicking on a link still fires an update on dcc.Location (rather than reloading the entire page), i.e. that the preventDefault is working as expected
  • As for the regular link behaviour, like right clicking on them, I'm not sure how we should test.

Let's publish a prerelease version and test this on the dash-docs page. To create a prerelease:
1 - Update version.py to be 0.23.1rc1
2 - Update package.json to be 0.23.1-rc1 (notice how the rc syntax is different between node and python. npm requires a - between the version number and the prerelease tag while python's pip just has 0.23.1rc1)
3 - Update __init__.py to reference the new node version:


should be 0.23.1-rc1
4 - Publish: npm run publish-all (I'll give you access to NPM and PyPI)
5 - Create a PR in dash-docs by editing the requirements.txt to contain the new prerelease version (0.23.1rc1): https://github.com/plotly/dash-docs/blob/master/requirements.txt#L8. In that repo. PRs automatically deploy a staged versions of the app. Tag me in the PR so that I can take a look as well!
6 - Comment on this thread when the prerelease version has been published, so that community members can take it for a spin as well 🚗

@chriddyp chriddyp referenced this pull request Jun 22, 2018

Merged

Confirmation modal #211

@valentijnnieman

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2018

@chriddyp I've written a test that should test if the page scrolls back up (should verify snapshot on Percy), if it updates the Location, and tests if the tag that is rendered by Link contains an href attribute with the correct href. My thinking is that if this is the case, then the tag should/must be right-clickable in browsers.

edit: So Percy takes a snapshot of the entire page of course, making my test for checking if scrolled to the top a bit useless. Any suggestions on how to test this properly?

edit-2: I've updated the version for prerelease, but unfortunately can't seem to publish. You do not have permission to publish "dash-core-components". Are you logged in as the correct user? : dash-core-components did you add me to the NPM team?

@valentijnnieman

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2018

I've published a prerelease version of this: 0.23.1rc1 and made a PR to the Docs here so we can try it out.

@chriddyp

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

This looks awesome!
link-component

One thing that I notice is that the standard cmd-click to open a new tab doesn't seem to work. Here's a GIF. The first couple of clicks are standard <a/> links, the last click to the gallery is a dcc.Link
link-component-cntrl-click

I'd be curious to know why that was the case. It's not a blocker for me as this is already a lot better

@chriddyp

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

It looks like the scroll issues are also fixed! Here's the current version on master:
link-component-scroll

And here's the new version:
link-component-scroll-new-version

👏 👏 👏 👏 👏 👏 👏 👏 👏 👏

@@ -2,6 +2,10 @@
All notable changes to this project will be documented in this file.
This project adheres to [Semantic Versioning](http://semver.org/).

## [0.23.1]
### fixed
- Link component now is a proper <a> tag so you can right click on it, and will scroll back to top. [#99](https://github.com/plotly/dash-core-components/issues/99)

This comment has been minimized.

Copy link
@chriddyp

chriddyp Jul 10, 2018

Member

Maybe say "Fixes #99, implemented in #215"

Link component now is a proper tag. This allows you to right-click on dcc.Link and see the standard html link context menu. Also, clicking on a dcc.Link will now scroll the next page to the top, rather than keeping the scroll position at the position of the previous page.

@app.callback(Output('page-content', 'children'),
[Input('test-url', 'pathname')])
def display_page(pathname):
return 'You are on page {}'.format(pathname)

This comment has been minimized.

Copy link
@chriddyp

chriddyp Jul 10, 2018

Member

Let's increment a multiprocessing.Value in here so that we can ensure that the callback is only fired a single time.
Here's an example of incrementing Value in a callback:
https://github.com/plotly/dash-renderer/blob/70bb93e8f0151ddc7d0810f3df271036a069360a/tests/test_render.py#L458-L463

and further down, the assertion:
https://github.com/plotly/dash-renderer/blob/70bb93e8f0151ddc7d0810f3df271036a069360a/tests/test_render.py#L478-L484

if (refresh) {
window.location.pathname = href;
} else {
window.history.pushState({}, '', href);
window.dispatchEvent(new CustomEvent('onpushstate'));
}
window.scrollTo(0, 0); // scroll back to top

This comment has been minimized.

Copy link
@chriddyp

chriddyp Jul 10, 2018

Member

Very sweet. For some reason I thought we would need to do this after the content was updated, but after playing around with it seems like this totally works 👏

@chriddyp
Copy link
Member

left a comment

This is really awesome. Two minor suggestions w.r.t. the CHANGELOG.md and the tests. After those are considered 💃 and feel free to merge and release

@ned2

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2018

So happy that these issues are being fixed!!

@valentijnnieman valentijnnieman merged commit 3fc5312 into master Jul 12, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
percy/dash-core-components Visual review automatically approved, no visual changes found.
Details

@valentijnnieman valentijnnieman deleted the 99-semantic-links branch Jul 12, 2018

@ned2 ned2 referenced this pull request Sep 1, 2018

Closed

Clean up the Dash tutorial #105

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.