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

setting save success, scroll to notification #18638

Merged
merged 10 commits into from Jan 18, 2022
Merged

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Jan 17, 2022

Description:

Fixes: #18637
after setting save success, should scroll to notification

Review

update plugin scroll
@peterhashair peterhashair added this to the 4.7.0 milestone Jan 17, 2022
@@ -227,7 +227,7 @@ export default defineComponent({
context: 'success',
type: 'transient',
});
NotificationsStore.scrollToNotification('generalSettings');
Copy link
Member

Choose a reason for hiding this comment

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

w/ the new code the way to do this is:

const notificationInstanceId = NotificationsStore.show({
  message: translate('CoreAdminHome_PluginSettingsSaveSuccess'),
  id: 'generalSettings',
  context: 'success',
  type: 'transient',
});
NotificationsStore.scrollToNotification(notificationInstanceId);

The reason for this change is because Notification.vue makes an AJAX request in markNotificationAsRead() if an ID is specified, but that isn't needed for notifications added client side only. So if you want to avoid making that request, you don't have to specify the id: property.

Copy link
Member

Choose a reason for hiding this comment

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

I'll add documentation for this later unless it's desired to revert this to the old behavior.

cc @sgiehl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@diosmosis that makes more sense. updating it now

Peter and others added 2 commits January 18, 2022 12:50
Copy link
Member

@diosmosis diosmosis left a comment

Choose a reason for hiding this comment

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

looks good if tests pass

@peterhashair peterhashair modified the milestones: 4.7.0, 4.8.0 Jan 18, 2022
# Conflicts:
#	plugins/CorePluginsAdmin/vue/dist/CorePluginsAdmin.umd.js
#	plugins/CorePluginsAdmin/vue/dist/CorePluginsAdmin.umd.min.js
@peterhashair
Copy link
Contributor Author

@diosmosis sorry to bother got a question. when I run ./console vue:build it returns random errors on 4.x-dev.
image

Also here, should that be a catch method instead of finally?

@peterhashair peterhashair merged commit 7feaf35 into 4.x-dev Jan 18, 2022
@peterhashair peterhashair deleted the m-18637-plug-scroll branch January 18, 2022 06:19
@diosmosis
Copy link
Member

diosmosis commented Jan 18, 2022

when I run ./console vue:build it returns random errors on 4.x-dev.

I've noticed this, seems to be a tsc issue/bug, but I don't know how to fix it. Currently just rebuilding when it happens. It doesn't happen very often for me (once a day at most).

Also here, should that be a catch method instead of finally?

It should be finally, unless you want to show the loading gif if there's an error. (EDIT: "no error" not an error.)

@peterhashair
Copy link
Contributor Author

@diosmosis I saw someone solved it by add this https://www.npmjs.com/package/eslint-import-resolver-typescript

@diosmosis
Copy link
Member

Thanks, @peterhashair! Do you have info on how they used it, specifically w/ vue CLI?

@peterhashair
Copy link
Contributor Author

@diosmosis haven't try myself, but they claim work like this nklayman/vue-cli-plugin-electron-builder#1059

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Saving general settings successfully no longer scrolls to the success notification
2 participants