Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Merges tabs into frames object in windowStore #8833

Merged
merged 1 commit into from
May 12, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented May 11, 2017

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Resolves #8689

Auditors: @bsclifton @bbondy

Test Plan:

  • tests should be green
  • check if dnd is working correctly
  • check if tabs are rendering correctly
  • check if audio icon, favicon, private icon is displayed correctly

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run indivually or as a suite ref

Resolves brave#8689

Auditors: @bsclifton @bbondy

Test Plan:
- tests should be green
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

There is also a ui.tabs object in windowStore that you may consider dumping in a follow up. It's used for storing the preview index. For an example see:
https://github.com/brave/browser-laptop/blob/master/app/sessionStore.js#L126

Left some comments which could also be done in a follow up. Otherwise, looks great 😄 👍

)}>
<Favicon
isActive={this.props.isActive}
paintTabs={this.props.paintTabs}
tab={this.props.tab}
frame={this.props.frame}
Copy link
Member

Choose a reason for hiding this comment

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

It might be better to create a reference for this (ex: const frame = this.props.frame) and then just pass the frame={frame}, instead of this.props.frame each time. This way, if it changes again, you only change one place

Copy link
Member

Choose a reason for hiding this comment

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

Same with all the stuff above; getting rid of all the duplicate this.props.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we will remove all this props in redux PR, so I think it's not necessary to do it right now.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM 👍 😄

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.

2 participants