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

Modify the "New Git Integration" experience to align with provider terminology #4287

Merged
merged 4 commits into from
Jun 11, 2021

Conversation

jordanhailey
Copy link
Contributor

Per #4275, submitting same code as #4285 on a shorter branch name to fix build error.

@jordanhailey jordanhailey changed the title Jordanhailey/issue 4275 Modify the "New Git Integration" experience to align with provider terminology May 24, 2021
@gtsiolis
Copy link
Contributor

gtsiolis commented May 24, 2021

/werft run

👍 started the job as gitpod-build-jordanhailey-issue-4275-fork.0

@jordanhailey jordanhailey marked this pull request as ready for review May 24, 2021 22:50
Copy link
Contributor Author

@jordanhailey jordanhailey left a comment

Choose a reason for hiding this comment

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

This may be beyond the scope of the issue, feel free to reject and I'll remove the hook.

Comment on lines +482 to +489
useEffect(() => {
if (props.mode === "new") {
// If the host value has been modified e.g. not gitlab.example.com, assume it has been set by user and end operation
if (!host.includes(".example.com")) return;
const exampleHostname = `${type.toLowerCase()}.example.com`;
updateHostValue(exampleHostname);
}
}, [type]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This hook is for a little UX improvement to modify host and redirectURL whenever the provider changes, that being said, I understand the team is following the Minimal Valuable Change approach and perhaps this creeps a bit beyond the scope of the issue I set out to address. If that's the case I am happy to remove the hook.

@csweichel
Copy link
Contributor

csweichel commented May 25, 2021

/werft run

👍 started the job as gitpod-build-jordanhailey-issue-4275-fork.1

@corneliusludmann
Copy link
Contributor

Hey @jordanhailey!

Thanks a lot for your contribution and sorry that it took so long to get feedback. We are still working on getting better at this. 😞

I really like your contribution and I am about to approve and merge this. Unfortunately, we need a signed CLA first for legal reasons and this process is not automated yet. 🙄 @meysholdt will contact you for this.

Thanks for your patience!

@jordanhailey
Copy link
Contributor Author

Thanks @corneliusludmann, I'm glad you like my small contribution. I got the docusign and executed the agreement on my end.

Cheers

@meysholdt
Copy link
Member

hi @jordanhailey, I have received the signed CLA. Thank you!

Copy link
Contributor

@corneliusludmann corneliusludmann left a comment

Choose a reason for hiding this comment

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

Thanks again for your contribution, @jordanhailey! 👍

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.

None yet

6 participants