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-7985 - Adapt sidebar active selection to rooms changes #3390

Merged
merged 14 commits into from
Sep 18, 2024

Conversation

uidp
Copy link
Contributor

@uidp uidp commented Sep 2, 2024

Short Description

In order to distinguish between the room details and course room details in the sidebar selection we have to add some specific logic.

Links to Ticket and related Pull-Requests

BC-7985

Changes

Data-security

Deployment

New Repos, NPM packages or vendor scripts

Screenshots of UI changes

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

Copy link
Contributor

@MartinSchuhmacher MartinSchuhmacher left a comment

Choose a reason for hiding this comment

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

als some tests seem to be failing in the CI

src/modules/ui/layout/sidebar/SidebarItem.vue Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Sep 18, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
75.3% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

}
},
{ immediate: true }
);

const isRoom = computed(() => roomVariant.value === RoomVariant.ROOM);
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure how I feel about roomVariant and RoomVariant working together here. Can we find a more distinctive naming?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, we can talk about that. I just wanted to avoid having two boolean flags.

@uidp uidp enabled auto-merge (squash) September 18, 2024 07:38
@@ -0,0 +1 @@
<template>&nbsp;</template>
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this about? Can this be removed again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the root route to prevent Vue warnings about missing route for path "". This page is actually never rendered. The &nbsp; is there because we cannot have empty <template> tags.

Copy link
Contributor

Choose a reason for hiding this comment

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

lets talk tomorrow about the whole idea for the routing so we can all have a similar understanding with the new approach :)

@uidp uidp merged commit de54286 into main Sep 18, 2024
70 of 71 checks passed
@uidp uidp deleted the BC-7985-sidebar-selection branch September 18, 2024 07:55
CeEv pushed a commit that referenced this pull request Sep 18, 2024
* implement selective sidebar selection for /rooms/:id

* fix sidebar selection for board

* move sidebar selection to composable

* add/fix tests
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.

3 participants