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

Hermes console vue move offsets #1708

Merged
merged 8 commits into from
Aug 28, 2023

Conversation

moscicky
Copy link
Collaborator

No description provided.

@moscicky moscicky assigned moscicky and unassigned moscicky Aug 18, 2023
@moscicky moscicky force-pushed the hermes_console_vue_move_offsets branch from e0a4869 to deceba1 Compare August 22, 2023 09:34
szczygiel-m
szczygiel-m previously approved these changes Aug 22, 2023
import { useI18n } from 'vue-i18n';

const props = defineProps<{
subscription: Subscription;
Copy link
Contributor

Choose a reason for hiding this comment

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

What do You think about passing just topicName and subscriptionName instead the whole Subscription object?


const msg = ref<string>();

function moveOffsets(): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function should be in composables

$t('subscription.moveOffsets.button')
}}</v-btn>
<!-- TODO: show a popup-->
<p class="text-wrap">{{ msg }}</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

For now, we can omit error/success messages printing. I think that it is better to add this as notification (included in Adrian's subscription form PR). Also lets omit confirmation popups for now, we will add this in separate PR.

validateStatus: (status: number) => {
return status > 200 && status < 400;
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I thinking right, that lines 131-135 are not necessary?

@szczygiel-m szczygiel-m dismissed their stale review August 22, 2023 12:53

missclick

@moscicky moscicky force-pushed the hermes_console_vue_move_offsets branch from deceba1 to f23a6d4 Compare August 24, 2023 14:35
@moscicky moscicky force-pushed the hermes_console_vue_move_offsets branch from f23a6d4 to c320a7e Compare August 28, 2023 07:31
@moscicky moscicky merged commit 0d705a1 into hermes_console_vue Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants