SED-4496 plan environment selection can never be disabled#1326
Conversation
Summary of ChangesHello @neogucky, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where plan environment selection parameters could not be effectively cleared or disabled. It introduces a mechanism to filter out undefined execution parameters, ensuring that only active selections are maintained. Additionally, it refines the user interface for setting these parameters by reorganizing explanatory text and updates the core popover component to provide more flexible and consistent positioning. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements. The popover component is refactored to support dynamic positioning, which also fixes a bug with duplicated position definitions. The default position of the popover is also updated. In the plan editor actions, the user experience for setting target execution parameters is enhanced by moving the help text into the action menu. Additionally, a change is made to filter out undefined execution parameters, which seems to address the core issue mentioned in the PR title. My review includes a suggestion to further simplify the new positioning logic and a fix for a typo in a user-facing text.
| private buildPositions(): ConnectedPosition[] { | ||
| const isBelow = this.yPosition() === 'below'; | ||
| const preferLeft = this.xPosition() === 'before'; | ||
|
|
||
| const buildPosition = ( | ||
| originY: 'top' | 'bottom', | ||
| overlayY: 'top' | 'bottom', | ||
| overlayX: 'start' | 'end', | ||
| ): ConnectedPosition => ({ | ||
| originX: 'center', | ||
| originY, | ||
| overlayX, | ||
| overlayY, | ||
| }); | ||
|
|
||
| const primary = buildPosition(isBelow ? 'bottom' : 'top', isBelow ? 'top' : 'bottom', preferLeft ? 'end' : 'start'); | ||
|
|
||
| const secondary = buildPosition( | ||
| isBelow ? 'bottom' : 'top', | ||
| isBelow ? 'top' : 'bottom', | ||
| preferLeft ? 'start' : 'end', | ||
| ); | ||
|
|
||
| const fallbackPrimary = buildPosition( | ||
| isBelow ? 'top' : 'bottom', | ||
| isBelow ? 'bottom' : 'top', | ||
| preferLeft ? 'end' : 'start', | ||
| ); | ||
|
|
||
| const fallbackSecondary = buildPosition( | ||
| isBelow ? 'top' : 'bottom', | ||
| isBelow ? 'bottom' : 'top', | ||
| preferLeft ? 'start' : 'end', | ||
| ); | ||
|
|
||
| return [primary, secondary, fallbackPrimary, fallbackSecondary]; | ||
| } |
There was a problem hiding this comment.
The buildPositions method can be simplified by extracting the repeated conditional logic into variables. This improves readability and maintainability by making the construction of the different position strategies more explicit and less repetitive.
private buildPositions(): ConnectedPosition[] {
const isBelow = this.yPosition() === 'below';
const preferLeft = this.xPosition() === 'before';
const primaryOriginY = isBelow ? 'bottom' : 'top';
const primaryOverlayY = isBelow ? 'top' : 'bottom';
const primaryOverlayX = preferLeft ? 'end' : 'start';
const secondaryOverlayX = preferLeft ? 'start' : 'end';
const buildPosition = (
originY: 'top' | 'bottom',
overlayY: 'top' | 'bottom',
overlayX: 'start' | 'end'
): ConnectedPosition => ({
originX: 'center',
originY,
overlayX,
overlayY,
});
return [
buildPosition(primaryOriginY, primaryOverlayY, primaryOverlayX),
buildPosition(primaryOriginY, primaryOverlayY, secondaryOverlayX),
buildPosition(primaryOverlayY, primaryOriginY, primaryOverlayX),
buildPosition(primaryOverlayY, primaryOriginY, secondaryOverlayX),
];
}
...rc/lib/modules/plan-editor/components/plan-editor-actions/plan-editor-actions.component.html
Outdated
Show resolved
Hide resolved
…plan-editor-actions/plan-editor-actions.component.html Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
neogucky
deleted the
SED-4496-plan-environment-selection-can-never-be-disabled
branch
* SED-4496 Correctly disable plan editor environment when unset * SED-4496 improved target execution parameters button * Update projects/step-frontend/src/lib/modules/plan-editor/components/plan-editor-actions/plan-editor-actions.component.html --------- Co-authored-by: Tim Rasim <2033832+neogucky@users.noreply.github.com>
1 participant
No description provided.