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

Fix#968/users page #970

Closed
wants to merge 18 commits into from
Closed

Conversation

m0ksem
Copy link
Collaborator

@m0ksem m0ksem commented Nov 14, 2023

Description

closes #968

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement/refactoring (non-breaking change that doesn't add any feature but make things better)

@m0ksem m0ksem changed the base branch from master to develop November 14, 2023 01:24
@m0ksem m0ksem requested a review from asvae November 14, 2023 01:24
Copy link
Member

@asvae asvae left a comment

Choose a reason for hiding this comment

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

Thanks for your work!

I'd say about 80% done.

src/pages/users/widgets/InactiveUsersTable.vue Outdated Show resolved Hide resolved
src/pages/users/widgets/UsersTable.vue Outdated Show resolved Hide resolved
src/pages/users/UsersPage.vue Outdated Show resolved Hide resolved
src/pages/users/widgets/EditUserForm.vue Outdated Show resolved Hide resolved
src/stores/inactive-users.ts Outdated Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
src/pages/users/UsersPage.vue Outdated Show resolved Hide resolved
src/pages/users/widgets/EditUserForm.vue Outdated Show resolved Hide resolved
src/pages/users/widgets/EditUserForm.vue Outdated Show resolved Hide resolved
src/pages/users/widgets/EditUserForm.vue Show resolved Hide resolved
@m0ksem m0ksem requested a review from asvae November 22, 2023 03:00
Copy link
Member

@asvae asvae left a comment

Choose a reason for hiding this comment

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

Apart from what I found - looks ready.

notes: 'Lorem ipsum dolor sit amet consectetur adipisicing elit. Quisquam, voluptatum.',
},
{
id: 7,
Copy link
Member

Choose a reason for hiding this comment

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

Let's add some more variety, maybe 10 users on repeat?

image

Also maybe only nice images, letters are placeholders and we don't have to cover all cases.

Copy link
Member

Choose a reason for hiding this comment

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

Almost there. Images are great, but when you open the page you see this:

image

It would be great to see this instead:

image

Can we maybe remove John Doe, Martin Hoff etc users without images?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Users without images were used in design, and I'd keep them as an example.

email: 'maksim@epic.com',
username: 'maksim',
role: 'admin',
projects: 1,
Copy link
Member

Choose a reason for hiding this comment

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

image

Let's use project names.
I'm fine without objects, but the way it is right now - I can't say the page is finished. So let's make it look good in this PR, then perhaps move to objects in #984 pr.

Copy link
Member

Choose a reason for hiding this comment

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

I mean something like the list below, number is not meaningful

"NexaFlow Manager"
"OrbitTrack Suite"
"DataPulse Portal"
"InnoSight Platform"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't have user role specific for project in data, but I can display projects which user a part of.
image

src/data/pages/users.ts Show resolved Hide resolved
src/data/pages/users.ts Outdated Show resolved Hide resolved
src/data/pages/users.ts Outdated Show resolved Hide resolved
src/data/pages/users.ts Show resolved Hide resolved
src/data/pages/users.ts Outdated Show resolved Hide resolved
@m0ksem m0ksem mentioned this pull request Nov 29, 2023
@asvae
Copy link
Member

asvae commented Nov 29, 2023

I think some things still need improvement. Coming primarily from user experience perspective.

@m0ksem
Copy link
Collaborator Author

m0ksem commented Dec 1, 2023

Move to #985 because we need both projects and users pages at the same time.

@m0ksem m0ksem closed this Dec 1, 2023
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.

2 participants