Add Git Integrations #3582
Add Git Integrations #3582
Conversation
AlexTugarev
force-pushed
the
at/git-init2
branch
from
50a4ce7 to
881b95a
Compare
There was a problem hiding this comment.
Superb @AlexTugarev! Thanks for pushing this through the finish line. Left some minor style issues. Feel free to merge once these have been resolved.
| </Modal> | ||
| )} | ||
|
|
||
| <h3 className="flex-grow self-center">Git Integration</h3> |
There was a problem hiding this comment.
nitpick: Minor typo.
| <h3 className="flex-grow self-center">Git Integration</h3> | |
| <h3 className="flex-grow self-center">Git Integrations</h3> |
There was a problem hiding this comment.
question: This minor typo is still missing. Do you think it looks better without this suggestion?
|
/werft run 👍 started the job as gitpod-build-at-git-init2.8 |
| setClientSecret(props.provider.oauth.clientSecret); | ||
| setRedirectURL(props.provider.oauth.callBackUrl); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
As to our discussion yesterday the commonly used pattern here is this facebook/react#15523 (comment)
const [type, setType] = useState<string>(props.provider.type);
...
userEffect(() => {
setType(props.provider.type);
...
}, [props.provider]);
There was a problem hiding this comment.
thanks for sharing!
in this case, the GitIntegrationModal component can be used in two modes, with and without a prop.provider to allow creating and editing.
| const [errorMessage, setErrorMessage] = useState<string | undefined>(); | ||
| const [validationError, setValidationError] = useState<string | undefined>(); | ||
|
|
||
| const firstRender = useRef(true); |
There was a problem hiding this comment.
Tracking how often it is rendered looks like you tried to fix something weird.
What's the purpose of firstRender? If you think this is the right solution, please at least explain in a comment.
There was a problem hiding this comment.
why so?
the first render thingy seems to be a common case, https://stackoverflow.com/questions/53253940/make-react-useeffect-hook-not-run-on-initial-render
the effect here is, that I don't want the validation on [clientId, clientSecret] being triggered right away, but only on user changes.
AlexTugarev
force-pushed
the
at/git-init2
branch
4 times, most recently
from
2545734 to
4254d4a
Compare
|
Done with review comments so far 🤞🏻 and squashed and rebased. |
| const validate = () => { | ||
| const errors: string[] = []; | ||
| if (clientId.trim().length === 0) { | ||
| errors.push("Client ID is missing."); |
There was a problem hiding this comment.
suggestion: What do you think of hiding the error message when you are trying to add a new integration and showing this only after clicking Activate Integration?
There was a problem hiding this comment.
But you can no longer click that button, as both values are missing.
There was a problem hiding this comment.
Yes, but there's no need to also show the error alert component, right? We could use an info alert component here if you think this is still helpful. Feel free to leave this as is for this iteration.
There was a problem hiding this comment.
Looks great @AlexTugarev! Left two minor comments in #3582 (comment) and #3582 (comment). Feel free to open a follow up issue for this if needed.
| @@ -15,7 +15,7 @@ export interface Props { | |||
| } | |||
|
|
|||
| export function SettingsPage(p: Props) { | |||
| return <div className="max-w-6xl"> | |||
| return <div className="w-full"> | |||
There was a problem hiding this comment.
issue: This also affected other settings pages that don't need to span across the right side, see /account. Could we bring back this padding for pages like /account? Happy to discuss this in a follow up issue. /cc @AlexTugarev
There was a problem hiding this comment.
All right. I'll revert.
There was a problem hiding this comment.
note: Integrations page should use the full width as is. Only accounts and other pages without much information should be limited to the middle. /cc @AlexTugarev
closes #3333
AlexTugarev
force-pushed
the
at/git-init2
branch
from
4254d4a to
73c48f6
Compare
Second part of #3401
Closes #3574
Git Integrations
Git Providers
Bugfixes