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
Enhancements/add profile image component #120
Enhancements/add profile image component #120
Conversation
…thub.com/JayyajGH/iznik-nuxt into enhancements/add-profile-image-component
Do we need the name property on the component?
You're setting a class of profilesm/profilemd on this component. I
think it would be better to indicate the size via a size prop, as
bootstrap does and we do in some other components, and then apply the
relevant class within the component.
…------ Original Message ------
From: "JayyajGH" <notifications@github.com>
To: "Freegle/iznik-nuxt" <iznik-nuxt@noreply.github.com>
Cc: "Subscribed" <subscribed@noreply.github.com>
Sent: 30/12/2019 15:04:49
Subject: [Freegle/iznik-nuxt] Enhancements/add profile image component
(#120)
This is the first stage of creating the profile component to sort out
some of the broken image issues. There are quite a few parts to this -
updating quite a few components and moving some styles around - so I
thought I would check it in a bit at a time so the change doesn't go
stale before I finish it and become a merge nightmare.
If you're happy with this change then go ahead and merge it and I'll
work through using this everywhere as time permits.
--------------------------------------------------------------------------------
You can view, comment on, or merge this pull request online at:
#120
Commit SummaryAdd a new component for profile images and use it for
news componentsAdd a new component for profile images and use it for
news componentsMerge branch 'enhancements/add-profile-image-component'
of https://github.com/JayyajGH/iznik-nuxt into
enhancements/add-profile-image-componentFile
ChangesMcomponents/NewsReply.vue
<https://github.com/Freegle/iznik-nuxt/pull/120/files#diff-0> (24)
Mcomponents/NewsThread.vue
<https://github.com/Freegle/iznik-nuxt/pull/120/files#diff-1> (14)
Acomponents/ProfileImage.vue
<https://github.com/Freegle/iznik-nuxt/pull/120/files#diff-2> (80)
Patch
Links:https://github.com/Freegle/iznik-nuxt/pull/120.patchhttps://github.com/Freegle/iznik-nuxt/pull/120.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#120?email_source=notifications&email_token=AAAN26CNKMXI67RDPEJCP7LQ3IEZDA5CNFSM4KBNMBKKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IDLM2HQ>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAN26GWMUQASRSMTQCP2HTQ3IEZDANCNFSM4KBNMBKA>.
|
…omponent' into enhancements/add-profile-image-component
Many moons ago I read somewhere that every component should have a name property set. That might have changed now though... I'll look into it... Yup that would be a good plan. When I did this initially I just tried to keep the changes to a minimum but I'll update that. There are probably a number of other things that could be refactored out too which will be clearer to see once all places are using the component. |
I think this is where I read about always adding a name to a component: |
Looks like there is a current PR to add a check for a name property to eslint too... |
Good spot on the name. I'd better do that, I guess.
…------ Original Message ------
From: "JayyajGH" <notifications@github.com>
To: "Freegle/iznik-nuxt" <iznik-nuxt@noreply.github.com>
Cc: "Edward Hibbert" <edward@ehibbert.org.uk>; "Comment"
<comment@noreply.github.com>
Sent: 30/12/2019 16:07:41
Subject: Re: [Freegle/iznik-nuxt] Enhancements/add profile image
component (#120)
Looks like there is a current PR to add a check for a name property to
eslint too...
vuejs/eslint-plugin-vue#945
<vuejs/eslint-plugin-vue#945>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#120?email_source=notifications&email_token=AAAN26FP2ZAAGR34HM7KQETQ3IME3A5CNFSM4KBNMBKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEH2T2WA#issuecomment-569720152>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAN26G3PJGCKTG3L73IQOLQ3IME3ANCNFSM4KBNMBKA>.
|
…nhancements/add-profile-image-component
…thub.com/JayyajGH/iznik-nuxt into enhancements/add-profile-image-component
Is this still WIP or waiting for merge? I'm not sure if you did the size prop. |
I've not done the size prop yet. I have work work to get done today so I'll take a look on Friday. Is there any way to convert a PR to a draft PR? |
Ok @edwh - This should now be ready to merge. I'll then sort all the others out on Friday. |
This is the first stage of creating the profile component to sort out some of the broken image issues. There are quite a few parts to this - updating quite a few components and moving some styles around - so I thought I would check it in a bit at a time so the change doesn't go stale before I finish it and become a merge nightmare.
If you're happy with this change then go ahead and merge it and I'll work through using this everywhere as time permits.