-
Notifications
You must be signed in to change notification settings - Fork 3
Show user name in content list header #235
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay for user types! 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smaller PRs are 1000% great and easier to review, thank you for splitting up this work!
const props = defineProps<{ | ||
user: User; | ||
}>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since ContentList
already makes use of stores, is there a reason to create a prop and not import userStore
directly here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do this to help with typing.
App.vue
has a loading spinner while the vulnerability store, content store, and user store are loading in data. The ContentList
only renders when the user store has a user, so if I pass it as a prop I can tell this component that it is guaranteed to have the data. If I use userStore.user
directly I will have to either have undefined
checks or use the TypeScript non-null assertion operator (!
)
let name; | ||
|
||
if (props.user.first_name && props.user.last_name) { | ||
name = `${props.user.first_name} ${props.user.last_name}`; | ||
} else if (props.user.first_name) { | ||
name = props.user.first_name; | ||
} else if (props.user.last_name) { | ||
name = props.user.last_name; | ||
} | ||
|
||
if (name === undefined) { | ||
name = props.user.username; | ||
} | ||
|
||
return `${name}'s Connect Content`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto from previous PRs, what about encapsulating this within the store to something like userStore.headerDisplayLabel
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah if I do this I'm realizing that I'll have to handle undefined
and I lose the guaranteed nature of using prop.user
. I'll leave this as is, but if I need the user's name in a display format in other places in the future I'll move it to the store.
This PR shows the user's name in the content list header in the Package Vulnerability Scanner making it clear whose content you are viewing.
It will use first name + last name if both are available, falling back to one or the other, and finally the user name if neither are set.
Fixes #210
Preview
These changes have been published to our internal Dogfood server here.