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

feat(frontend): show open rooms sidebar #1273

Merged
merged 2 commits into from
Jul 5, 2024
Merged

feat(frontend): show open rooms sidebar #1273

merged 2 commits into from
Jul 5, 2024

Conversation

Elweyn
Copy link
Member

@Elweyn Elweyn commented Jun 28, 2024

🍰 Pullrequest

Issues

Todo

  • None

@Elweyn Elweyn self-assigned this Jun 28, 2024
@Elweyn Elweyn force-pushed the 709-merge-master branch from 4d084b9 to d288980 Compare July 2, 2024 14:40
@Elweyn Elweyn enabled auto-merge (squash) July 2, 2024 14:56
Copy link
Member Author

@Elweyn Elweyn left a comment

Choose a reason for hiding this comment

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

Top and BottomMenu have duplicate code.
Unit Test:

  • for ListElement should test the functionality
  • for SearchField should test the functionality
  • for ListWithNavigationDrawer test the functionality

Copy link
Contributor

@Bettelstab Bettelstab left a comment

Choose a reason for hiding this comment

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

(I don't want it to auto-merge, so I request changes)
Please see comments.
I can agree to do the refactoring and addition of more tests in a subsequent PR, but would suggest to do the smaller fixes right now before merging?

@@ -3,30 +3,54 @@
</template>

<script lang="ts">
import { defineComponent, provide } from 'vue'
import { DefaultApolloClient } from '@vue/apollo-composable'
import { defineComponent, provide } from 'vue'
Copy link
Contributor

Choose a reason for hiding this comment

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

Unintended indent change?

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that /storybook is in .eslintignore, I don't see why and suggest we change it.

frontend/src/components/menu/BottomMenu.vue Show resolved Hide resolved
frontend/src/components/menu/BottomMenu.vue Show resolved Hide resolved
frontend/src/components/menu/TopMenu.vue Show resolved Hide resolved
import Circle from './CircleElement.vue'
import LightDarkSwitch from './LightDarkSwitch.vue'
import LogoImage from './LogoImage.vue'
import TabControl from './TabControl.vue'
import UserInfo from './UserInfo.vue'

const drawer = ref(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

"Just" naming, but I have a strong preference for isDrawerVisible, or maybe showDrawer. I figured that Vuetify internally calls this drawer as well, but that doesn't make it a better name imo.

Comment on lines +51 to +61
const emits = defineEmits(['update:modelValue'])
const internalValue = ref(props.modelValue)
watch(internalValue, (newValue) => {
emits('update:modelValue', newValue)
})
watch(
() => props.modelValue,
(newValue) => {
internalValue.value = newValue
},
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const emits = defineEmits(['update:modelValue'])
const internalValue = ref(props.modelValue)
watch(internalValue, (newValue) => {
emits('update:modelValue', newValue)
})
watch(
() => props.modelValue,
(newValue) => {
internalValue.value = newValue
},
)
const internalValue = defineModel<string>()

@@ -0,0 +1,69 @@
<template>
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this could be merged with ListElement.vue to RoomList.vue and we could remove a lot of code.

Otherwise:

  • Remove in fact unused location property
  • Use defineModel() syntax
  • Rename drawer to something more meaningful

frontend/src/stores/activeRoomStore.ts Show resolved Hide resolved
frontend/src/components/menu/TopMenu.vue Show resolved Hide resolved
frontend/src/components/menu/BottomMenu.vue Show resolved Hide resolved
@Bettelstab
Copy link
Contributor

I can't comment in the code, because there was no change. But in DefaultLayout.vue line 17, we should remove the class="d-md-none" to avoid a warning, as BottomMenu doesn't have a single root element anymore to pass on the class property. The class is included in that file now, which is fine.
Besides, we could think about refactoring the code so that ListWithNavigationDrawer is only used once and not in both menus in the same way.

Comment on lines +53 to +56
.create-button-mobile {
z-index: 1;
transform: translate(20px, 30px);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I think this should not be here.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right if this is needed it should be in an own PR.

Elweyn added 2 commits July 5, 2024 10:43
Merge master and resolve conflicts.

remove timeout for refetch rooms

silent refresh without default layout

no default lazyut on auth page

more logs, use setRooms to update state

delay refetch open rooms 1 min, more console to debug

console to debug

linting

error handling refetch open rooms

handle click event on open room

type startTime in Room/OpenRoom is string

quick and dirty fixing of List Drawer, open rooms query, rooms store

Add rooms store.

Merge branch 'master' into 709-6-Toggle-In-TopMenu
Update snapshots.

Fix linting

Add function of ListNavigationDrawer.

Test remove handleOpenRooms.

fix style

Merge branch 'master' into 709-6-Toggle-In-TopMenu
fix eslint

remove space

lint fix

resolve conflicts

remove render files

remove only from test

resolve conflicts

change classname

fix test, fix lint

resolve conflicts

fix test and lint and update

add snapshot

resolve conflicts, remove Navigatin Drawer Component

remove unused prop

Update frontend/src/components/vuetify/NavigationDrawer.vue

Co-authored-by: Moriz Wahl <moriz.wahl@gmx.de>
Update frontend/src/components/vuetify/NavigationDrawer.vue

Co-authored-by: Moriz Wahl <moriz.wahl@gmx.de>
add snapshots

remove unused file

add locale, change tsconfig

coverage to 96%

Merge branch 'master' into 709-6-Toggle-In-TopMenu
Merge branch 'master' into 709-6-Toggle-In-TopMenu
fix lint

add snapshots, ??render config???

add toggle on camera icon, open sibebar

fix lint, fix test

clear imports

Add Snapshots

clear imports

Merge branch '709-7-SearchField-Component' into 709-4-ListWithNavigationDrawer-Component

Add Snapshot, fix lint

Add SearchField Component

fix clear imports

 change files and folders for atomic design

add ListWithNavigationDrawer Component, change files and folders for atomic design

Merge branch '709-5-NavigationDrawer-Component' into 709-4-ListWithNavigationDrawer-Component

fix test

add snapshot

fix lint

NavigationDrawer Component

fix test and lint

Add ListElement Component
@Elweyn Elweyn force-pushed the 709-merge-master branch from 05d4f2c to 4a15437 Compare July 5, 2024 08:44
@Bettelstab Bettelstab self-requested a review July 5, 2024 09:50
@Elweyn Elweyn merged commit 58af99d into master Jul 5, 2024
68 checks passed
@mahula mahula deleted the 709-merge-master branch July 25, 2024 07:26
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.

🚀 [Feature] Frontend: Sidebar to show open BBB rooms
2 participants