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

User preferences for Create/Edit method #9830

Merged
merged 2 commits into from Aug 27, 2021
Merged

User preferences for Create/Edit method #9830

merged 2 commits into from Aug 27, 2021

Conversation

nemesis09
Copy link
Contributor

@nemesis09 nemesis09 commented Aug 18, 2021 •
edited

Epic:
https://issues.redhat.com/browse/ODC-5227

Story:
https://issues.redhat.com/browse/ODC-5959

Analysis/Root cause:
As a user, I need the ability to set/edit my preferences for Create/Edit method (form or yaml) through user preferences.

Solution/Description:
This PR uses the dynamic extension added in #9386 to contribute user preference for Create/Edit method.

  • the preferredCreateEditMethod can be set from User Preferences page
  • preferredCreateEditMethod will be used for default option when SyncedEditorField loads
  • if preferredCreateEditMethod is not defined, lastViewedEditorType for the coomponent will be used from user settings
  • if both preferredCreateEditMethod and lastViewedEditorType are not defined, the initial value passed to formik field will be used
  • every component using SyncedEditorField will have its own user setting for lastViewedEditorType

Screens:
Screenshot from 2021-08-18 22-31-06
Screenshot from 2021-08-18 22-31-14

Test Coverage:
Screenshot from 2021-08-19 21-02-28

Browser conformation:

  • Firefox
  • Chrome
  • Safari
  • Edge

@openshift-ci openshift-ci bot added the component/core Related to console core functionality label Aug 18, 2021
@openshift-ci openshift-ci bot added component/dev-console Related to dev-console component/helm Related to helm-plugin component/knative Related to knative-plugin component/pipelines Related to pipelines-plugin component/sdk Related to console-plugin-sdk component/shared Related to console-shared component/topology Related to topology kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Aug 18, 2021
@nemesis09
Copy link
Contributor Author

This PR is dependent on #9386 for the dynamic extensions

@nemesis09
Copy link
Contributor Author

/retest

@nemesis09 nemesis09 force-pushed the user-preferences-form-yaml branch 2 times, most recently from 5dd36e2 to a4819fc Compare August 19, 2021 15:31
@nemesis09
Copy link
Contributor Author

/retest

1 similar comment
@nemesis09
Copy link
Contributor Author

/retest

@nemesis09 nemesis09 force-pushed the user-preferences-form-yaml branch 2 times, most recently from 5fd7e5b to cbdff26 Compare August 19, 2021 16:55
@openshift-ci openshift-ci bot added the component/olm Related to OLM label Aug 19, 2021
rohitkrai03
rohitkrai03 suggested changes Aug 19, 2021
View reviewed changes
frontend/packages/console-shared/src/components/formik-fields/SyncedEditorField.tsx Outdated
@@ -29,6 +35,7 @@ type SyncedEditorFieldProps = {
name: string;
formContext: EditorContext<SanitizeToForm>;
yamlContext: EditorContext<SanitizeToYAML>;
lastViewedEditorTypeUserSettingKey: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep the prop name shorter? Maybe just userSettingsKey?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could we use lastViewUserSettingKey?
WDYT?

frontend/packages/console-shared/src/components/formik-fields/SyncedEditorField.tsx Outdated
return field.value as EditorType;
};

const [activeEditorType, setActiveEditorType] = React.useState<EditorType>(getEditorType);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need another state for editor type? Wouldn't the formik state value work here?

Copy link
Contributor Author

@nemesis09 nemesis09 Aug 20, 2021 •
edited

Choose a reason for hiding this comment

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

updated to custom hook

frontend/packages/console-shared/src/components/formik-fields/SyncedEditorField.tsx Outdated
@@ -120,7 +155,15 @@ const SyncedEditorField: React.FC<SyncedEditorFieldProps> = ({
setDisabledFormAlert(formContext.isDisabled);
}, [formContext.isDisabled]);

return (
React.useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need this effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need this to update the last viewed user setting when the component loads.
this becomes useful when the last viewed type is different from the preferred type.

frontend/packages/console-shared/src/components/formik-fields/SyncedEditorField.tsx Outdated
setLastViewedEditorType(activeEditorType);
setFieldValue(name, activeEditorType);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment why the linting is disabled here.

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

frontend/packages/console-shared/src/components/synced-editor/index.tsx Outdated
Comment on lines 51 to 72
const [
lastViewedEditorType,
setLastViewedEditorType,
lastViewedEditorTypeLoaded,
] = useUserSettings<EditorType>(lastViewedEditorTypeUserSettingKey);
const [preferredEditorType, preferredEditorTypeLoaded] = usePreferredCreateEditMethod();

const getEditorType = (): EditorType => {
if (
preferredEditorType &&
preferredEditorType !== PREFERRED_CREATE_EDIT_METHOD_USER_SETTING_VALUE_LATEST
) {
return preferredEditorType as EditorType;
}
if (lastViewedEditorType) {
return lastViewedEditorType;
}
return initialType;
};
const [type, setType] = React.useState<EditorType>(getEditorType());

const loaded = preferredEditorTypeLoaded || lastViewedEditorTypeLoaded;
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like most of this can be extracted out into a separate hook since its being used in SyncedEditorField as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as per discussion, updated to a custom hook.

@openshift-ci openshift-ci bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 20, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2021
@nemesis09
Copy link
Contributor Author

/retest

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

5 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2021
@invincibleJai
Copy link
Member

/retest

@invincibleJai
Copy link
Member

/retest-required

@openshift-ci openshift-ci bot added the kind/cypress Related to Cypress e2e integration testing label Aug 26, 2021
@invincibleJai
Copy link
Member

/retest-required

@nemesis09
Copy link
Contributor Author

/retest

@invincibleJai invincibleJai mentioned this pull request Aug 27, 2021
Closed
jerolimov
jerolimov reviewed Aug 27, 2021
View reviewed changes
frontend/packages/console-shared/src/components/synced-editor/useEditorType.ts
React.useEffect(() => {
if (resourceLoaded) {
const editorType: EditorType = getEditorType();
setLastViewedEditorType(editorType);
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid setting the last viewed editor type when resources are just loaded.

Just because the user opens a form, its not required to save the current state. We should wrap this with an if state if this is different then before OR better, just don't do this. This is always a patch network call, a kubernetes write operation, just because the user opens a form. Does it also work if we just skip this line?

Suggested change
setLastViewedEditorType(editorType);

Copy link
Member

Choose a reason for hiding this comment

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

I will raise a bug for this

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 is required when the lastEditorType doesn't exist or when it is different from the preferredEditorType only when the form loads

@jerolimov
Copy link
Member

jerolimov commented Aug 27, 2021 •
edited

Its working fine. I will create a bug (ODC-6302) that we should avoid unnecessary API calls.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 27, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: invincibleJai, jerolimov, nemesis09, rohitkrai03

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nemesis09
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit 5301b1e into openshift:master Aug 27, 2021
@yozaam yozaam mentioned this pull request Aug 30, 2021
Merged
4 tasks
@spadgett spadgett added this to the v4.9 milestone Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jerolimov jerolimov jerolimov left review comments

@rohitkrai03 rohitkrai03 rohitkrai03 approved these changes

@andrewballantyne andrewballantyne Awaiting requested review from andrewballantyne

@christianvogt christianvogt Awaiting requested review from christianvogt

Assignees

@jerolimov jerolimov

@invincibleJai invincibleJai

@christianvogt christianvogt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dev-console Related to dev-console component/helm Related to helm-plugin component/knative Related to knative-plugin component/olm Related to OLM component/pipelines Related to pipelines-plugin component/sdk Related to console-plugin-sdk component/shared Related to console-shared component/topology Related to topology docs-approved Signifies that Docs has signed off on this PR kind/cypress Related to Cypress e2e integration testing kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
v4.9
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@nemesis09 @rohitkrai03 @invincibleJai @christianvogt @openshift-bot @jerolimov @spadgett @openshift-merge-robot