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

BC-7415 - Make sidebar usable on tablets #3285

Merged
merged 6 commits into from
Jun 18, 2024

Conversation

MartinSchuhmacher
Copy link
Contributor

@MartinSchuhmacher MartinSchuhmacher commented Jun 14, 2024

Short Description

  • sidebar not scrollable on tablet with first touch/scroll (vue)
  • scroll on main content not disabled, when sidebar is open (vue and legacy)
  • scroll on task item more menu (in task overview) not possible when "edit" option is touched (vue)

Links to Ticket and related Pull-Requests

BC-7415
hpi-schul-cloud/schulcloud-client#3457

Changes

(see Description)

Checklist before merging

  • QA: In addition to review, the code has been manually tested (if manual testing is possible)
  • PO: Any deviation from requirements was agreed with Product-Owner / ticket author / support-team
  • DEV: Every new component is implemented having accessibility in mind (e.g. aria-label, role property)

Notice: Please keep this Pull-Request as a Draft (or add WIP label), until it is ready to be reviewed

@MartinSchuhmacher MartinSchuhmacher marked this pull request as ready for review June 14, 2024 10:45
@@ -16,7 +16,7 @@
</v-list-item>
</template>
<template v-for="child in item.children" :key="child.title">
<SidebarItem :item="child" />
<SidebarItem :item="child" :draggable="false" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this draggable prop come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

vuedraggable uses that option to not make a component draggable

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm very curious about this, as the Sidebar doesn't use vuedraggable

const sidebarExpanded = ref(isDesktop.value);

const onToggleSidebar = () => {
sidebarExpanded.value = !sidebarExpanded.value;
};
</script>
<style lang="scss" scoped>
.fixed-position {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather use !isDesktop and vuetify's utility classes here position-fixed w-100, but it's fine :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uhh thats a good point, thx, will adjust :)

Copy link

sonarcloud bot commented Jun 18, 2024

@@ -16,7 +16,7 @@
</v-list-item>
</template>
<template v-for="child in item.children" :key="child.title">
<SidebarItem :item="child" />
<SidebarItem :item="child" :draggable="false" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm very curious about this, as the Sidebar doesn't use vuedraggable

@MartinSchuhmacher MartinSchuhmacher merged commit 295cf97 into main Jun 18, 2024
47 of 49 checks passed
@MartinSchuhmacher MartinSchuhmacher deleted the BC-7415-sidebar-on-mobile branch June 18, 2024 11:50
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.

2 participants