Skip to content

NavLink: set default href to JS:void, change callback args to not modify immutable synthetic event #484

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

Merged
merged 1 commit into from
Jul 26, 2016
Merged

NavLink: set default href to JS:void, change callback args to not modify immutable synthetic event #484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions components/global-navigation-bar/link.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const GlobalNavigationBarLink = (props) => {
const {
active,
className,
href,
label,
onClick,
...other
Expand All @@ -45,14 +46,16 @@ const GlobalNavigationBarLink = (props) => {
if (isFunction(onClick)) {
EventUtil.trap(event);

event.href = props.href;
onClick(event);
const hrefCheck = href === 'javascript:void(0);' ? undefined : href; // eslint-disable-line no-script-url

onClick(event, hrefCheck);
}
}

return (
<li className={classNames('slds-context-bar__item', { 'slds-is-active': active })}>
<a
href={href}
className={classNames('slds-context-bar__label-action', className)}
onClick={handleClick}
{...other}
Expand Down Expand Up @@ -89,4 +92,8 @@ GlobalNavigationBarLink.propTypes = {
onClick: PropTypes.func
};

GlobalNavigationBarLink.defaultProps = {
href: 'javascript:void(0);' // eslint-disable-line no-script-url
};

module.exports = GlobalNavigationBarLink;
15 changes: 1 addition & 14 deletions stories/global-navigation-bar/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ const getGlobalNavigationBar = (props, primaryRegionProps) => (
</GlobalNavigationBarRegion>
<GlobalNavigationBarRegion region="secondary" navigation>
<GlobalNavigationBarLink
href="#"
href="http://google.com"
label="Home"
id="home-link"
onClick={linkClicked('Home link clicked')}
Expand All @@ -56,18 +56,15 @@ const getGlobalNavigationBar = (props, primaryRegionProps) => (
options={dropdownCollection}
/>
<GlobalNavigationBarLink
href="#"
label="Menu Item 2"
onClick={linkClicked('Link clicked')}
/>
<GlobalNavigationBarLink
active
href="#"
label="Menu Item 3"
onClick={linkClicked('Link clicked')}
/>
<GlobalNavigationBarLink
href="#"
label="Menu Item 4"
onClick={linkClicked('Link clicked')}
/>
Expand Down Expand Up @@ -105,7 +102,6 @@ const getGlobalNavigationBarCustomCloud = (props, primaryRegionProps) => (
</GlobalNavigationBarRegion>
<GlobalNavigationBarRegion region="secondary" navigation>
<GlobalNavigationBarLink
href="#"
label="Overview"
id="overview-link"
onClick={linkClicked('Overview link clicked')}
Expand All @@ -129,7 +125,6 @@ const getGlobalNavigationBarCustomCloud = (props, primaryRegionProps) => (
options={dropdownCollection}
/>
<GlobalNavigationBarLink
href="#"
label="A/B Testing"
onClick={linkClicked('A/B Testing Link clicked')}
/>
Expand All @@ -140,19 +135,16 @@ const getGlobalNavigationBarCustomCloud = (props, primaryRegionProps) => (
options={dropdownCollection}
/>
<GlobalNavigationBarLink
href="#"
label="Admin"
onClick={linkClicked('Admin Link clicked')}
/>
<GlobalNavigationBarLink
href="#"
label="Audience Builder"
onClick={linkClicked('Audience Builder Link clicked')}
/>
</GlobalNavigationBarRegion>
<GlobalNavigationBarRegion region="tertiary">
<GlobalNavigationBarLink
href="#"
label="Actions"
onClick={linkClicked('Link clicked')}
/>
Expand Down Expand Up @@ -180,7 +172,6 @@ const getGlobalNavigationBarCustomCloudOverviewAcive = (props, primaryRegionProp
</GlobalNavigationBarRegion>
<GlobalNavigationBarRegion region="secondary" navigation>
<GlobalNavigationBarLink
href="#"
label="Overview"
id="overview-link"
active
Expand All @@ -205,7 +196,6 @@ const getGlobalNavigationBarCustomCloudOverviewAcive = (props, primaryRegionProp
options={dropdownCollection}
/>
<GlobalNavigationBarLink
href="#"
label="A/B Testing"
onClick={linkClicked('A/B Testing Link clicked')}
/>
Expand All @@ -216,19 +206,16 @@ const getGlobalNavigationBarCustomCloudOverviewAcive = (props, primaryRegionProp
options={dropdownCollection}
/>
<GlobalNavigationBarLink
href="#"
label="Admin"
onClick={linkClicked('Admin Link clicked')}
/>
<GlobalNavigationBarLink
href="#"
label="Audience Builder"
onClick={linkClicked('Audience Builder Link clicked')}
/>
</GlobalNavigationBarRegion>
<GlobalNavigationBarRegion region="tertiary">
<GlobalNavigationBarLink
href="#"
label="Actions"
onClick={linkClicked('Link clicked')}
/>
Expand Down
23 changes: 18 additions & 5 deletions tests/global-navigation-bar/global-navigation-bar.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,27 +194,40 @@ describe('Global Navigation Bar: ', () => {
// TODO still need Dropdown covered. Should be added to Dropdown tests, once special context bar dropdown features are merged into Dropdown

describe('GlobalNavigationLink child component', () => {
it('GlobalNavigationBarLink has attributes and onClick runs callback', function () {
const linkClicked = sinon.spy();
let linkClicked;
let link;

beforeEach(function () {
linkClicked = sinon.spy();
const instance = (
<GlobalNavigationBarLink
href="http://google.com"
label="Home"
id="home-link"
onClick={linkClicked('Home link clicked')}
/>
);
this.wrapper = mount(instance, { attachTo: document.body.appendChild(document.createElement('div')) });
const link = this.wrapper.find('#home-link');
link = this.wrapper.find('#home-link');
});

afterEach(function () {
this.wrapper.unmount();
});

it('GlobalNavigationBarLink has attributes and onClick runs callback', function () {
expect(link.text()).to.equal('Home');
link.simulate('click');
expect(linkClicked.calledOnce).to.be.true;
Copy link
Contributor

Choose a reason for hiding this comment

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

@iowillhoit I'm going to go ahead and merge this. Great work. Helped me with my PR too! 👍 I'll create another issue. We should probably test that the URL gets passed as a parameter..

});

this.wrapper.unmount();
it('renders href if passed', () => {
expect(link).to.have.attr('href', 'http://google.com');
});
});

describe('GlobalNavigationButton child component', () => {
it('GlobalNavigationBarLink has attributes and onClick runs callback', function () {
it('GlobalNavigationBarButton has attributes and onClick runs callback', function () {
const buttonClicked = sinon.spy();
const instance = (
<GlobalNavigationBarButton
Expand Down