Change groups at topbar home icon #2072
Conversation
|
Oh, two details that need fixing: |
|
To align groups without image, you could add a placeholder div with fixed size. I'm not concerned about listing inactive groups - show them if the users is still member (they get removed anyways after 7 months of inactivity) What about users that have only one group? I think the Karrot logo was a nice way to get back to the group home page. Of course, they could also use the sidenav menu to achieve the same. I just thought of having a dropdown-type of thing next to the Karrot/group logo - it would always show your current group (replacing that part of the breadcrumb). When you click on it, jump to group home page. When you click the dropdown-arrow on the right, show the group menu. What do you think about this? |
|
The snapshots seem to diverge a lot from One "story" also needs updating; you might have noticed the warning From groups: { getters: { all: () => groupsMock } },to groups: { getters: {
all: () => groupsMock,
mine: () => groupsMock,
} }, |
|
Thank you for the review, Tilmann! I believe the things you pointed out are fixed. However, I just noticed a little unexpected behavior that needs fixing also before merging. When going to the group gallery, all of groups in which the user's application is pending are appended to the list of groups on the topbar. |
Seems they should either be always there or never, regardless if the user visited the group gallery. It just affects the people that have an application pending, so it may not be many on production sites. Which solution would you prefer? Hide groups for which applications are pending? |
|
By the way, the linter wants some more newlines in the code I gave you, have a look at this: |
|
Probably not the most elegant solution, but everything seems to work now! You're welcome to change nonetheless. I'm just glad I made it work, but I can learn from other approaches. |
src/groupInfo/datastore/groups.js
Outdated
| @@ -41,6 +41,7 @@ export default { | |||
| return Object.values(state.entries).map(getters.enrich) | |||
| }, | |||
| mine: (state, getters) => getters.all.filter(e => isMyGroup(e)).sort(applicationsFirstThenSortByName), | |||
| isMemberGroups: (state, getters) => getters.all.filter(e => isMemberGroup(e)).sort(applicationsFirstThenSortByName), | |||
There was a problem hiding this comment.
Minor: you can simplify this:
.filter(e => isMemberGroup(e))to this:
.filter(isMemberGroup)Because e => isMemberGroup(e) is just a function that takes one argument (e) and calls one function (isMemberGroup), that's also the same as just the reference to the function isMemberGroup itself...
There was a problem hiding this comment.
Got it! I copied the line above, (e => isMyGroup(e)), so I changed both getters now to this simplified reference to the function
src/groupInfo/datastore/groups.js
Outdated
| @@ -41,6 +41,7 @@ export default { | |||
| return Object.values(state.entries).map(getters.enrich) | |||
| }, | |||
| mine: (state, getters) => getters.all.filter(e => isMyGroup(e)).sort(applicationsFirstThenSortByName), | |||
| isMemberGroups: (state, getters) => getters.all.filter(e => isMemberGroup(e)).sort(applicationsFirstThenSortByName), | |||
There was a problem hiding this comment.
Also minor.
I find the name isMemberGroups confusing, as normally something starting with is will return a boolean value (e.g. the isMemberGroup function).
I would probably call it mineWithoutApplications ... or maybe call it mine and rename the existing mine to be mineWithApplications.
There was a problem hiding this comment.
I went for mine and mineWithApplications. Hopefully I managed to change it wherever it is referenced. Nothing seems to be broken
|
Great work 👍 ... just the snapshot tests failing still... |
|
I merged master into it (rebase had conflicts) and updated the snapshots :) |
Closes #1746
What does this PR do?
Adds a group switcher to the topbar home icon
Links to related issues
Checklist