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

UI: add merge strategy #3581

Merged
merged 3 commits into from
Jun 30, 2022
Merged

UI: add merge strategy #3581

merged 3 commits into from
Jun 30, 2022

Conversation

eden-ohana
Copy link
Contributor

closes #3266

@eden-ohana eden-ohana added the exclude-changelog PR description should not be included in next release changelog label Jun 27, 2022
@eden-ohana eden-ohana added include-changelog PR description should be included in next release changelog and removed exclude-changelog PR description should not be included in next release changelog labels Jun 27, 2022
@eden-ohana eden-ohana marked this pull request as ready for review June 27, 2022 19:07
@itaiad200 itaiad200 requested review from johnnyaug and ozkatz June 28, 2022 14:59
@itaiad200
Copy link
Contributor

itaiad200 commented Jun 28, 2022

Added @johnnyaug + @ozkatz for product feedback on the ui

Copy link
Contributor

@johnnyaug johnnyaug left a comment

Choose a reason for hiding this comment

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

Great!
See comments.
Suggestion to make it less eye-catching - use a select dropdown instead of radio buttons.

<FormControl>
<FormLabel id="demo-radio-buttons-group-label">Merge Strategy</FormLabel>
<FormHelperText>
In case of a merge conflict, this option will force the merge process
Copy link
Contributor

Choose a reason for hiding this comment

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

Great description!

Comment on lines 219 to 222
In case of a merge conflict, this option will force the merge process
to automatically favor changes from the dest branch (&rdquo;dest-wins&rdquo;) or
from the source branch(&rdquo;source-wins&rdquo;). In case no selection is made,
the merge process will fail in case of a conflict.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: use concrete branch names (I also added a missing space there).

Suggested change
In case of a merge conflict, this option will force the merge process
to automatically favor changes from the dest branch (&rdquo;dest-wins&rdquo;) or
from the source branch(&rdquo;source-wins&rdquo;). In case no selection is made,
the merge process will fail in case of a conflict.
In case of a merge conflict, this option will force the merge process
to automatically favor changes from <b>{dest}</b> (&rdquo;dest-wins&rdquo;) or
from <b>{source}</b> (&rdquo;source-wins&rdquo;). In case no selection is made,
the merge process will fail in case of a conflict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

</FormHelperText>
<RadioGroup
aria-labelledby="demo-radio-buttons-group-label" defaultValue="none" name="radio-buttons-group" row>
<FormControlLabel value="none" control={<Radio />} label="none" onChange={onStrategyChange}/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<FormControlLabel value="none" control={<Radio />} label="none" onChange={onStrategyChange}/>
<FormControlLabel value="none" control={<Radio />} label="Default" onChange={onStrategyChange}/>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@eden-ohana eden-ohana requested a review from johnnyaug June 29, 2022 20:26
Copy link
Contributor

@johnnyaug johnnyaug left a comment

Choose a reason for hiding this comment

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

Yay

@eden-ohana eden-ohana merged commit 5d0df99 into master Jun 30, 2022
@eden-ohana eden-ohana deleted the 3266-merge-strategy-ui branch June 30, 2022 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Add Merge Strategy to the UI
3 participants