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

Stories rework: VaAvatar #3795

Merged
merged 3 commits into from
Sep 6, 2023
Merged

Stories rework: VaAvatar #3795

merged 3 commits into from
Sep 6, 2023

Conversation

Roman4437
Copy link
Collaborator

Avatar stories

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.

There are very small things. After fix - please merge on your own :).

components: { VaAvatar },
template: '<VaAvatar/>',
template: `
<VaAvatar>
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 use avatar with image as default example.

export const Color = () => ({
components: { VaAvatar },
template: `
[warning]
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem to be relevant. Same in other places.

Suggested change
[warning]

export const Size = () => ({
components: { VaAvatar },
template: `
[smal]
Copy link
Member

Choose a reason for hiding this comment

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

Here small/medium etc is fine, as we have several options.

export const FontSize = () => ({
components: { VaAvatar },
template: `
[0.75rem]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[0.75rem]

<VaAvatar
src="https://randomuser.me/api/portraits/women/5.jpg"
alt="image"
>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>
/>


export const FallbackText = () => ({
components: { VaAvatar },
template: '<VaAvatar src="https://not-exist" fallbackText="Fallback"/>',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
template: '<VaAvatar src="https://not-exist" fallbackText="Fallback"/>',
template: '<VaAvatar src="https://not-exist" fallbackText="Text"/>',

Current version looks like bug to me.
image

export const FallbackRender = () => ({
components: { VaAvatar },
methods: {
FallbackRender: () => h('b', {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
FallbackRender: () => h('b', {
fallbackRender: () => h('b', {

Methods start from lowercase.

},
}, 'Text'),
},
template: '<VaAvatar src="https://not-exist" :fallbackRender="FallbackRender"/>',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
template: '<VaAvatar src="https://not-exist" :fallbackRender="FallbackRender"/>',
template: '<VaAvatar src="https://not-exist" :fallbackRender="fallbackRender"/>',

@asvae asvae mentioned this pull request Sep 5, 2023
75 tasks
@Roman4437 Roman4437 merged commit 02d25e7 into epicmaxco:develop Sep 6, 2023
m0ksem pushed a commit that referenced this pull request Sep 12, 2023
* va-avatar stories

* va-avatar stories

* va-avatar stories
@Roman4437 Roman4437 deleted the va-avatar branch September 18, 2023 13:33
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