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 disable state for the links in Nav component #8120

Merged

Conversation

winperec
Copy link
Contributor

@winperec winperec commented Feb 26, 2019

Pull request checklist

Description of changes

I've added disable state for the Nav links. Design is done using within existing concepts. This is how it looks like:
image

Also please review how it work in action:
nav_disabled_link

Here we have example how disable works with

  • Links that have children links 'Home' link is disabled but expandable
  • Simple text links ('MSN' link)
  • Links with Icon

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

@msftclas
Copy link

msftclas commented Feb 26, 2019

CLA assistant check
All CLA requirements met.

@winperec winperec changed the title Issue 7835 nav link disable state Add disable state for the links in Nav component Feb 26, 2019
@size-auditor
Copy link

size-auditor bot commented Feb 26, 2019

Bundle test Size (minified) Diff from master
Nav 187.051 kB ExceedsBaseline     153 bytes

ExceedsTolerance  Exceeds Tolerance     ExceedsBaseline  Exceeds Baseline     BelowBaseline  Below Baseline     1 kB = 1000 bytes

@natalieethell
Copy link
Contributor

This is awesome @winperec! Thanks so much for tackling this work. :) Just one request, would you mind adding a visual regression test in Nav.stories.tsx?

It might look something like this:

.addStory(
    'Disabled',
    () => (
      <div style={{ width: '208px' }}>
        <Nav
          groups={[{ links: disabledLinks }]}
          expandedStateText={'expanded'}
          collapsedStateText={'collapsed'}
          selectedKey={'key3'}
        />
      </div>
    ),
    { rtl: true }
);

where disabledLinks is similar to what exists in the stories file already, with the appropriate disabled states.

@betrue-final-final FYI

Copy link
Contributor

@natalieethell natalieethell left a comment

Choose a reason for hiding this comment

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

Looks great! I left comments on some minor things.

apps/vr-tests/src/stories/Nav.stories.tsx Outdated Show resolved Hide resolved
{
name: 'Unavailable Item',
url: 'http://cnn.com',
icon: 'News',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this IconNames.News?

apps/vr-tests/src/stories/Nav.stories.tsx Outdated Show resolved Hide resolved
{
name: 'Unavailable Item',
url: 'http://cnn.com',
icon: 'News',
icon: IconNames.News,
Copy link
Contributor

Choose a reason for hiding this comment

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

You're right about the deprecation of IconNames, good catch. We can change this back to 'News' like you had before.

Suggested change
icon: IconNames.News,
icon: 'News',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The following solution can work for OTB constant icons
https://gist.github.com/winperec/052cba5e7fcb563ab92ccc8ae2f0b1bb
but I think Office UI Fabric supports registering custom icons and in this case it will not work :)

@winperec
Copy link
Contributor Author

@natalieethell Thanks for review! Looking forward to see it in next minor release and remove our local workarounds

@aneeshack4 aneeshack4 merged commit 7ddc1ee into microsoft:master Feb 28, 2019
@msft-github-bot
Copy link
Contributor

🎉[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable items in <Nav /> component
5 participants