Skip to content

Keeping organization when switching tabs on publish repository modal#6636

Closed
brendonbribeiro wants to merge 5 commits intodesktop:developmentfrom
brendonbribeiro:fix-tab-change
Closed

Keeping organization when switching tabs on publish repository modal#6636
brendonbribeiro wants to merge 5 commits intodesktop:developmentfrom
brendonbribeiro:fix-tab-change

Conversation

@brendonbribeiro
Copy link
Contributor

This is my first PR to the project.

closes #6546

@outofambit outofambit added the ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 15, 2019
@tierninho
Copy link
Contributor

Tested and LGTM however I could not test the Enterprise organization retention in the dev environment.

@billygriffin
Copy link
Contributor

Hi @brendonbarreto, thanks so much for submitting your first pull request in Desktop! 🎉 Our team is really focused on our 1.6 release scheduled to go out Thursday, so I don't anticipate a thorough review until after that. Welcome to the community and looking forward to hopefully getting this into the product. ❤️

@brendonbribeiro
Copy link
Contributor Author

Thanks for the welcome @billygriffin , and for the quick feedback!

@iAmWillShepherd iAmWillShepherd self-assigned this Jan 16, 2019
Copy link
Contributor

@iAmWillShepherd iAmWillShepherd left a comment

Choose a reason for hiding this comment

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

Not clearing out the org can create issues when the selection in the DotCom tab doesn't exist in the Enterprise tab. This change will require introducing some additional state to keep track of the selected org for each tab.

@brendonbribeiro
Copy link
Contributor Author

brendonbribeiro commented Jan 18, 2019

@iAmWillShepherd I made the changes you requested. Now each tab has its own state. Sorry for any mistake, I'm new to typescript world (but I already find it beautiful)

I was not able to test enterprise tab .

@iAmWillShepherd
Copy link
Contributor

@brendonbarreto Have a look at #6776. I added a bit of type safety to what is being done in this PR. If it looks good to you, I can either push to your branch or we can go with the new PR (you'd still get credit of course 👍 ).

@brendonbribeiro
Copy link
Contributor Author

@iAmWillShepherd I think we can go with the new PR (#6776)

@iAmWillShepherd
Copy link
Contributor

Closing due to #6636 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review Pull Requests that are ready to be reviewed by the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Organization selection should remain when toggling between Github and Enterprise

5 participants