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.

Sign up for GitHub

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

Jump to bottom

consolidate add-edit-duplicate modals #1732

Merged
merged 16 commits into from
Jul 22, 2024
Merged

consolidate add-edit-duplicate modals #1732

merged 16 commits into from
Jul 22, 2024

Conversation

danoswaltCL
Copy link
Collaborator

Opening as draft, is dependent on the backend updates Ben has made, but otherwise should be good for review.

This consolidates the ADD, EDIT, and DUPLICATE modals into a common template modal UpsertFeatureFlagModal, as they are nearly identical except for a couple of straightforward params.

This adds the DUPLICATE modal to the FF details menu. The only actual difference between ADD and DUPLICATE is the title and we add _COPY_CHANGE_ME to the name and key.

image image image

@danoswaltCL danoswaltCL requested review from bcb37, KD1712 and zackcl July 3, 2024 22:18
@Yagnik56 Yagnik56 self-requested a review July 4, 2024 02:51
Yagnik56
Yagnik56 previously approved these changes Jul 8, 2024
View reviewed changes
Copy link
Collaborator

@Yagnik56 Yagnik56 left a comment

Choose a reason for hiding this comment

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

It looks good to me, some questions and suggestions but nothing seems to be a blocker to not merge this PR.

frontend/projects/upgrade/src/assets/i18n/en.json Outdated Show resolved Hide resolved
frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.model.ts Show resolved Hide resolved
frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.model.ts Outdated Show resolved Hide resolved
frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.model.ts Outdated Show resolved Hide resolved
@Yagnik56 Yagnik56 self-requested a review July 12, 2024 08:39
@danoswaltCL danoswaltCL mentioned this pull request Jul 18, 2024
Closed
@danoswaltCL danoswaltCL marked this pull request as ready for review July 18, 2024 14:30
@danoswaltCL danoswaltCL requested a review from Yagnik56 July 18, 2024 14:30
@danoswaltCL
Copy link
Collaborator Author

This one is merged with backend changes and works like a charm now for Add - Edit - Duplicate, ready for review @zackcl @Yagnik56 @Yagnik56

@zackcl zackcl mentioned this pull request Jul 19, 2024
Merged
Yagnik56
Yagnik56 reviewed Jul 19, 2024
View reviewed changes
frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts Outdated Show resolved Hide resolved
frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts Show resolved Hide resolved
frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts Show resolved Hide resolved
frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts Show resolved Hide resolved
frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts Show resolved Hide resolved
frontend/projects/upgrade/src/assets/i18n/en.json Outdated Show resolved Hide resolved
frontend/projects/upgrade/src/app/shared/services/common-text-helpers.service.ts Outdated Show resolved Hide resolved
...hboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts Outdated
return { name, key, description, appContext, tags };
}

deriveName(sourceFlag: FeatureFlag, action: string): string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why derive values separately for every property?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what's your suggested code improvement?

Copy link
Collaborator

@Yagnik56 Yagnik56 Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

can u try this?

deriveInitialFormValues(sourceFlag: FeatureFlag, action: string) {
  return {
    name: action === UPSERT_FEATURE_FLAG_ACTION.EDIT ? sourceFlag?.name : '',
    key: action === UPSERT_FEATURE_FLAG_ACTION.EDIT ? sourceFlag?.key : '',
    description: sourceFlag?.description || '',
    appContext: sourceFlag?.context?.[0] || '',
    tags: sourceFlag?.tags || [],
  };
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's exactly what I'd like to avoid doing, what's the benefit in jamming all that in there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll ask in the dev mtg, I'm curious what others say, I guess I have a very different coding style than the rest of the team in general, I want to do what is comfortable for everyone.

...hboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts Outdated
}
}

createRequest(action: UPSERT_FEATURE_FLAG_ACTION, sourceFlag?: FeatureFlag): void {
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of createRequest, something like sendRequest?

Yagnik56
Yagnik56 reviewed Jul 22, 2024
View reviewed changes
frontend/projects/upgrade/src/assets/i18n/en.json Outdated Show resolved Hide resolved
...oard/feature-flags/modals/import-feature-flag-modal/import-feature-flag-modal.component.scss
}

a {
text-decoration: none;
Copy link
Collaborator

@Yagnik56 Yagnik56 Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

why is this removed? @zackcl The design show link shouldn't be underlined.

frontend/projects/upgrade/src/styles.scss

.learn-more-link {
color: var(--blue);
text-decoration: underline;
Copy link
Collaborator

Choose a reason for hiding this comment

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

in figma we don't have underlines in learn more redirect links, can you confirm @zackcl ?

...lone-component-lib/components/common-import-container/common-import-container.component.html
<mat-icon>file_upload</mat-icon>
<p class="ft-14-600 drag-text">Drag & drop or</p>
<button mat-flat-button color="primary" (click)="fileInput.click()">
<span class="material-symbols-outlined">upload</span>
Copy link
Collaborator

Choose a reason for hiding this comment

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

how are you able to get the upload icon here without mat-icon @zackcl just curious.

@danoswaltCL
Copy link
Collaborator Author

If this is good other than some styling can we merge? would be easier to complete the story i'm currently working on. @zackcl @Yagnik56

@Yagnik56
Copy link
Collaborator

Yagnik56 commented Jul 22, 2024 •
edited
Loading

If this is good other than some styling can we merge? would be easier to complete the story i'm currently working on. @zackcl @Yagnik56

can u resolve that deriveInitialFormValues function comment if possible, I can approve if you think it's fine this way?

@Yagnik56 Yagnik56 self-requested a review July 22, 2024 12:50
Yagnik56
Yagnik56 previously approved these changes Jul 22, 2024
View reviewed changes
@Yagnik56 Yagnik56 self-requested a review July 22, 2024 14:29
@danoswaltCL danoswaltCL merged commit 7c3f819 into dev Jul 22, 2024
8 checks passed
@danoswaltCL danoswaltCL deleted the feature/upsert-flag-common-modal-template branch July 22, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yagnik56 Yagnik56 Yagnik56 approved these changes

@bcb37 bcb37 Awaiting requested review from bcb37

@KD1712 KD1712 Awaiting requested review from KD1712

@zackcl zackcl Awaiting requested review from zackcl

Assignees

@danoswaltCL danoswaltCL

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@danoswaltCL @Yagnik56 @zackcl