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

New server modal warnings #442

Merged
merged 4 commits into from
Mar 16, 2017
Merged

Conversation

jnugh
Copy link
Contributor

@jnugh jnugh commented Feb 18, 2017

Before submitting, please confirm you've

Summary
Fixes multiple warnings that were introduced by the new team modal PR.

Fixes #440

Test Cases
Check if I changed the '+' Button in the TabBar (I had to wrap it correctly and had to make some CSS changes).

@jasonblais
Copy link
Contributor

Thanks @jnugh! Can you help submit it against the release-3.6 branch so we'll include it in the upcoming release?

@jasonblais jasonblais added this to the v3.6.0 milestone Feb 18, 2017
@jnugh jnugh changed the base branch from master to release-3.6 February 18, 2017 15:50
@jnugh jnugh force-pushed the newServerModalWarnings branch 2 times, most recently from ff027cf to 568253e Compare February 18, 2017 16:00
@jnugh
Copy link
Contributor Author

jnugh commented Feb 18, 2017

Changed the target and rebased the PR.

@jasonblais
Copy link
Contributor

Thanks @jnugh for the quick fixes!

  1. After refresh, one more JavaScript error remains, the other ones have cleared.

image

  1. The style of the "+" icon is slightly changed with:
  • overlapping the desktop tab bar divider line
  • not being quite centered relative to the desktop server name

image

PS: I found a couple of other JavaScript errors when clicking around, not sure if these are related?

  1. After clicking the "+" icon

image

  1. When going to the Settings page

image

  1. When clicking "Add new server" in Settings page

image

@jnugh jnugh force-pushed the newServerModalWarnings branch from 568253e to 29b176b Compare February 18, 2017 16:53
@jnugh
Copy link
Contributor Author

jnugh commented Feb 18, 2017

1 und 4 have been there for some time already.
2 and 5 is easy to fix.

  1. I have no idea right now - Didn't think of this. The button is now wrapped in a NavItem. Therefore a click on that button is forwarded to the tab listeners.
    This actually breaks multiple things a lot of things as we are not dealing with warnings but actual errors.

I have actually no idea how to wrap the button correctly in the navigation without breaking everything. @yuya-oc do you have an idea?

@yuya-oc
Copy link
Contributor

yuya-oc commented Feb 19, 2017

1
This would be gone by removing initial nodeIntegration attribute from webview. It's unknown, so reported as warning by React. Existing tests would check whether node integration is correctly disabled.

3
TabBar.prop.onSelect() calls back the index of the selected item. For example, When "+" is clicked, it would call back "2" when there are two servers. But MainPage.handleSelect listener doesn't take care this.
As the result, the error message appears because MainPage.componentDidUpdate() tries to refer mattermostView2 which doesn't exist yet.

To fix this, I feel that adding onClickAdd prop to TabBar to stop using Button.onClick and handle Nav.onSelect to call back onAddServer is proper.

function handleSelect(key) {
  if (key === this.props.teams.length) {
    this.props.onAddServer();
  } else {
    this.props.onSelect(key);
  }
}

<Nav onSelect={handleSelect}>

Or using Button.onClick and:

function handleSelect(key) {
  if (key < this.props.teams.length) {
    this.props.onSelect(key);
  }
}

<Nav onSelect={handleSelect}>

4
It's shown because our components are not completely stateless. I think it's not easy to fix soon.

@jasonblais
Copy link
Contributor

Thanks! I can create tickets for 1 and 4, no need to worry about those for this PR

@jasonblais
Copy link
Contributor

@jnugh not sure if 3 is something you have time to work on?

@jasonblais jasonblais modified the milestones: v3.7.0, v3.6.0 Feb 26, 2017
@yuya-oc yuya-oc changed the base branch from release-3.6 to master March 1, 2017 15:21
@jnugh jnugh force-pushed the newServerModalWarnings branch from 359e3aa to eb76688 Compare March 15, 2017 19:22
@jnugh
Copy link
Contributor Author

jnugh commented Mar 15, 2017

I simply added a key to the NavItem and do a check in the handler. Works fine for me :)
Very sorry for the delay to get the last commit in.

@yuya-oc
Copy link
Contributor

yuya-oc commented Mar 16, 2017

Thanks for updating!

I feel it's wired that MainPage knows about internal key of TabBar. However it's an easy way for now.

After adding a server, the console shows an error. But it's not for this PR.

image

@jasonblais
Copy link
Contributor

Looks good! Created a separate issue for the console error that shows after adding a server #468

@yuya-oc yuya-oc merged commit b705415 into mattermost:master Mar 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants