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

[MS] User details modal #6000

Merged
merged 1 commit into from
Dec 18, 2023
Merged

[MS] User details modal #6000

merged 1 commit into from
Dec 18, 2023

Conversation

Ironicbay
Copy link
Contributor

@Ironicbay Ironicbay commented Dec 15, 2023

Describe your changes

Closes #5974

Checklist

Before you submit this pull request, please make sure to:

  • Keep changes in the pull request as small as possible
  • Ensure the commit history is sanitized
  • Give a meaningful title to your PR
  • Describe your changes
  • Link any related issue in the description
  • Link any dependent pull request in the description

If this PR adds a new feature or fixes a bug:

  • I have added or updated relevant tests

If this PR is related to GUI:

  • I have checked that GUIs affected by my changes are working properly.

@Ironicbay Ironicbay requested a review from a team as a code owner December 15, 2023 12:59
@Ironicbay Ironicbay marked this pull request as draft December 15, 2023 13:02
@Ironicbay Ironicbay self-assigned this Dec 15, 2023
@Ironicbay Ironicbay marked this pull request as ready for review December 15, 2023 14:13
@Ironicbay Ironicbay force-pushed the ms-user-details-modal branch 3 times, most recently from dfdd86a to 733bd52 Compare December 15, 2023 15:49
.user-details-modal {
--width: 642px;
--height: 468px;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

where is it coming from? copy/paste? is there a specific reflexion behind these values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These values are taken from the mockups, I was unsure if setting them explicitly was necessary or not (and now that you mention it, the height value is different in the revoked mockup so I'm guessing it's not)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes in lots of cases the modal will fits its content and it will be good enough.
@fabienSvtr a thought on this?

Copy link
Contributor

@fabienSvtr fabienSvtr Dec 18, 2023

Choose a reason for hiding this comment

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

We need to open an issue to harmonise all modal width. For this PR you can let the specific value.
I recently updated the Figma with width variables for that.
image

client/src/views/users/UserContextMenu.vue Outdated Show resolved Hide resolved
client/src/views/users/UserDetailsModal.vue Outdated Show resolved Hide resolved
client/src/views/users/UserDetailsModal.vue Outdated Show resolved Hide resolved
client/src/views/users/UserContextMenu.vue Outdated Show resolved Hide resolved
client/src/views/users/UserContextMenu.vue Outdated Show resolved Hide resolved
client/tests/e2e/specs/test_user_details_modal.ts Outdated Show resolved Hide resolved
client/tests/e2e/specs/test_user_details_modal.ts Outdated Show resolved Hide resolved
client/tests/e2e/specs/test_user_details_modal.ts Outdated Show resolved Hide resolved
client/src/views/users/UserDetailsModal.vue Outdated Show resolved Hide resolved
client/src/views/users/UserDetailsModal.vue Outdated Show resolved Hide resolved
client/src/views/users/UserDetailsModal.vue Outdated Show resolved Hide resolved
client/tests/e2e/specs/test_user_details_modal.ts Outdated Show resolved Hide resolved
Comment on lines 9 to 26
<div>
<ion-text class="text1">
{{ $t('UsersPage.UserDetailsModal.subtitles.name') }}
</ion-text>
<ion-text class="text1-right">
{{ $t('UsersPage.UserDetailsModal.subtitles.joined') }}
</ion-text>
</div>
<div>
<ion-text class="text2">
{{ user.humanHandle.label }}
</ion-text>
<ion-text class="text2-right">
{{ timeSince(user.createdOn, '--', 'short') }}
</ion-text>
</div>
<div>
<ion-text class="text4">
{{ $t('UsersPage.UserDetailsModal.subtitles.commonWorkspaces') }}
</ion-text>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Here the BEM suggestion.
In addition, it is preferable to group the code according to context so that each group/block can be managed (such as the user's name and title).

Suggested change
<div>
<ion-text class="text1">
{{ $t('UsersPage.UserDetailsModal.subtitles.name') }}
</ion-text>
<ion-text class="text1-right">
{{ $t('UsersPage.UserDetailsModal.subtitles.joined') }}
</ion-text>
</div>
<div>
<ion-text class="text2">
{{ user.humanHandle.label }}
</ion-text>
<ion-text class="text2-right">
{{ timeSince(user.createdOn, '--', 'short') }}
</ion-text>
</div>
<div>
<ion-text class="text4">
{{ $t('UsersPage.UserDetailsModal.subtitles.commonWorkspaces') }}
</ion-text>
</div>
<div class="detail-name">
<ion-text class="details-name__title">
{{ $t('UsersPage.UserDetailsModal.subtitles.name') }}
</ion-text>
<ion-text class="detail-name__fullname">
{{ user.humanHandle.label }}
</ion-text>
</div>
<div class="detail-joined">
<ion-text class="detail-joined__title">
{{ $t('UsersPage.UserDetailsModal.subtitles.joined') }}
{{ user.humanHandle.label }}
</ion-text>
<ion-text class=class="detail-joined__date">
{{ timeSince(user.createdOn, '--', 'short') }}
</ion-text>
</div>
<div>
<ion-text class="text4">
{{ $t('UsersPage.UserDetailsModal.subtitles.commonWorkspaces') }}
</ion-text>
</div>

client/src/views/users/UserDetailsModal.vue Outdated Show resolved Hide resolved
client/src/views/users/UserDetailsModal.vue Outdated Show resolved Hide resolved
client/src/views/users/UserDetailsModal.vue Outdated Show resolved Hide resolved
@Ironicbay Ironicbay force-pushed the ms-user-details-modal branch 3 times, most recently from 5fd1a1d to ca5e388 Compare December 18, 2023 14:27
@Ironicbay Ironicbay added this pull request to the merge queue Dec 18, 2023
Merged via the queue into master with commit a6625fa Dec 18, 2023
13 checks passed
@Ironicbay Ironicbay deleted the ms-user-details-modal branch December 18, 2023 15:42
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.

User details modal
4 participants