Conversation
pymnh
requested review from
max-nextcloud,
mejo- and
silverkszlo
as code owners
max-nextcloud
left a comment
max-nextcloud
left a comment
There was a problem hiding this comment.
Thanks a lot @pymnh !
Code looks good. Design as well. Great idea to follow what's in the dashboard widget.
I did not test it myself though.
mejo-
left a comment
mejo-
left a comment
There was a problem hiding this comment.
Thanks a lot, I agree that it's a great idea to have this button in the members dialog, especially as long as we don't have a list of related other resources for the team in question within the Collectives app.
Your approach to squeeze the button into the header of NcDialog using CSS doesn't work well though, see my comment below.
| } | ||
|
|
||
| .team-link { | ||
| position: absolute; |
There was a problem hiding this comment.
Unfortunately this doesn't work well. The button is hidden below the "close dialog" button:
I think a better approach would be to migrate CollectiveMembersModal back from NcDialog to NcModal which allows to customize all the contents of the modal, including its header.
See the code of NcDialog for a suggestion how to rebuild the component using NcModal but keeping it similar to how it looks now with NcDialog (except for the added button in the header).
There was a problem hiding this comment.
Thanks for the feedback. I will look into that :)
There was a problem hiding this comment.
I migrated the the CollectiveMembersModal to NcModal as you suggested.
I also used the freedom that the migration gives to make the whole title a hyperlink to the team overview, instead of only the icon, but I can revert that if you think that is too opinionated (Making only the name a link such as Members of collective<a>{name}[^]</a> does not seem possible as it would break the positioning due to different translations).
Full disclosure: The css is mostly AI generated with a bit of manual adjustment and some sanity-checking if it matches with the NcDialog layout.
Also seems to work on mobile:
But place it in the members modal next to the name of the collective, instead of bloating the landing page. Signed-off-by: Peymaneh <peymaneh@posteo.net>
pymnh
force-pushed
the
feat/bring-back-team-overview-link
branch
from
b51504f to
4f5d70d
Compare
Signed-off-by: Peymaneh <peymaneh@posteo.net>
pymnh
force-pushed
the
feat/bring-back-team-overview-link
branch
from
4f5d70d to
eb17bd7
Compare
The error with the psalm check seems to be unrelated to the changes, but because of the branch living on a fork? |
| min-height: var(--default-clickable-area); | ||
| overflow-wrap: break-word; | ||
| margin-block: 0 12px; | ||
| margin-inline: 0; |
There was a problem hiding this comment.
This seems superfluous and also doesn't exist in NcDialog sources. AI artifact? 😉
| padding-bottom: 12px; | ||
|
|
||
| &__name { | ||
| font-size: 21px; |
There was a problem hiding this comment.
A short comment that these rules are copied over from NcDialog would be good I think.
| display: inline; | ||
| vertical-align: middle; | ||
| color: var(--color-text-maxcontrast); | ||
| white-space: nowrap; |
There was a problem hiding this comment.
I don't think setting white-space has an impact on an icon-only element.
|
|
||
| .team-link { | ||
| color: var(--color-main-text); | ||
| text-decoration: none; |
There was a problem hiding this comment.
text-decoration seems superfluous as there's already none without setting it.
| @close="onClose"> | ||
| <div class="modal-collective-members"> | ||
| <h2 class="modal-collective-members__name"> | ||
| <a |
There was a problem hiding this comment.
To be honest I don't think it's a good idea to turn the whole heading into a link. People will hover over it accidentally and don't get where this link leads. I'd prefer to just have the button next to it being a link.
3 participants
📝 Summary
Initially I wanted to open a ticket to remove the Teams overview from the collectives landing page and move it into the members modal, because I too think that it causes too much confusion there and belongs in the members modal. My wish came partially true in 8c9e829 but now there is no direct link from the collective to its corresponding team overview, which I personally found to be useful.
This PR brings back the link to the team overview that was removed in 8c9e829 , but places it in the members modal next to the name of the collective. This is more intuitive then having it on the landing page: I am already in the members overview and am offered to go "deeper" into the team configuration. The link icon is generally understood as leading to a new page/modal.
🖼️ Screenshots
Notes
The usage of the OpenInNewIcon follows the design of the Teams dashboard widget:
🏁 Checklist
npm run lint/npm run stylelint/composer run cs:check)has been updated oris not required