-
Notifications
You must be signed in to change notification settings - Fork 14
WIP: BC-10453 Minor a11y fixes #3847
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
base: main
Are you sure you want to change the base?
Conversation
<script setup lang="ts"> | ||
import { ModelRef, PropType } from "vue"; | ||
export type VCustomDialogButton = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check if other usages of VCustomDialog need to be refactored to this type, too.
`[data-testid="share-modal-external-tools-info"]` | ||
); | ||
|
||
expect(infotext.isVisible()).toBe(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this invisible-check removed intentionaly?
src/components/share/ShareModal.vue
Outdated
@next="onNext()" | ||
<v-dialog | ||
v-model="isOpen" | ||
data-testid="sharedialog" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that changing the data-testid does not have impact on e2e-tests for example... or just keep it as it was.
src/components/share/ShareModal.vue
Outdated
v-model="isOpen" | ||
data-testid="sharedialog" | ||
max-width="480" | ||
@after-leave="onCleanUp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep the naming
@after-leave="onCleanUp" | |
@after-leave="onCloseDialog" |
as the name should describe the event not the actions that are taken in the case of the event.
{{ | ||
t("components.molecules.share.options.tableHeader.InfoText") | ||
}} | ||
<ul class="ml-6"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the combinations tested - to ensure that the right list items are shown for each setting being true - and "no setting true"?
src/components/share/ShareModal.vue
Outdated
<share-modal-result | ||
:share-url="shareUrl" | ||
:type="type" | ||
@done="onCloseDialogOrDone" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above eventName.
}; | ||
const onNext = () => { | ||
// shareModule.createShareUrl({ isSchoolInternal: false, hasExpiryDate: false }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comments
src/components/share/ShareModal.vue
Outdated
const showRoomInfo = computed(() => { | ||
return props.type === "room"; | ||
const listItems = computed(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The readabilty in the template code was increased... but this config-part is not (very much) more readable and reduces extendability...
<v-card data-testid="copy-info-dialog"> | ||
<UseFocusTrap> | ||
<v-card-title> | ||
<h2 class="mt-2 text-break"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While merging main into this branch, the title was changed to h2.
We should check if we need px-6 pt-4
|
Short Description
Links to Ticket and related Pull-Requests
https://ticketsystem.dbildungscloud.de/browse/BC-10453
Checklist before merging