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.

Sign up for GitHub

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

Jump to bottom

Move sharing indicators into own component and add extension point #2928

Merged
merged 1 commit into from
Jan 30, 2020
Merged

Move sharing indicators into own component and add extension point #2928

merged 1 commit into from
Jan 30, 2020

Conversation

LukasHirt
Copy link
Collaborator

@LukasHirt LukasHirt commented Jan 28, 2020 •
edited
Loading

Description

Move sharing indicators into smaller components, enable disabling of default indicators and add extension point for new indicators.

Related Issue

How Has This Been Tested?

  • test environment: Manually
  1. Disable default indicators inside of theme
  2. Create new indicators (example https://github.com/LukasHirt/phoenix-status-indicators) or add a custom indicators into already existing extension

Screenshots (if appropriate):

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 change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Add changelog item
  • Implement click handlers for default indicators

@LukasHirt LukasHirt self-assigned this Jan 28, 2020
@LukasHirt LukasHirt changed the title Move sharing indicators into own component and add extension point [WIP] Move sharing indicators into own component and add extension point Jan 28, 2020
@LukasHirt LukasHirt changed the title [WIP] Move sharing indicators into own component and add extension point Move sharing indicators into own component and add extension point Jan 29, 2020
@LukasHirt LukasHirt force-pushed the feature/themeable-sharing-indicators branch 2 times, most recently from a0f68f3 to 2f4bfff Compare January 29, 2020 14:22
@LukasHirt
Copy link
Collaborator Author

Tests are still failing, will investigate more and there is one error in console regarding vuex mutations which I'm digging deeper in now. I think you can do a "first round" of the review though @PVince81

@LukasHirt
Copy link
Collaborator Author

Seems that the error is coming from within the extension when the item prop is used. Not yet sure why. Continuing 🔎

PVince81
PVince81 reviewed Jan 29, 2020
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

See comments.

Can you also extend "apps/files/docs/extensionpoints.adoc" and put a quick example code there to illustrate how a custom indicator implementation looks like ?

From reading the code I could only briefly infer that the custom indicators are separate vue components but am not clear how the code would look like.

apps/files/src/components/AllFilesList.vue Outdated Show resolved Hide resolved
apps/files/src/components/FilesLists/StatusIndicators/DefaultIndicators.vue Show resolved Hide resolved
apps/files/src/components/FilesLists/StatusIndicators/DefaultIndicators.vue Show resolved Hide resolved
apps/files/src/components/FilesLists/StatusIndicators/StatusIndicators.vue Show resolved Hide resolved
changelog/unreleased/2895 Outdated Show resolved Hide resolved
@LukasHirt
Copy link
Collaborator Author

Can you also extend "apps/files/docs/extensionpoints.adoc" and put a quick example code there to illustrate how a custom indicator implementation looks like ?

I completely missed that we have such a file until now 🙈

@LukasHirt LukasHirt force-pushed the feature/themeable-sharing-indicators branch 2 times, most recently from 5f4b760 to a354a02 Compare January 30, 2020 10:35
@LukasHirt LukasHirt requested a review from PVince81 January 30, 2020 10:36
@LukasHirt LukasHirt added Status:Needs-Review Needs review from a maintainer and removed Status:In-Progress labels Jan 30, 2020
PVince81
PVince81 reviewed Jan 30, 2020
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Code looks good but example is not self-explanatory enough at first glance

@LukasHirt LukasHirt force-pushed the feature/themeable-sharing-indicators branch 2 times, most recently from ca62893 to ceabe83 Compare January 30, 2020 12:22
@LukasHirt
Copy link
Collaborator Author

LukasHirt commented Jan 30, 2020 •
edited
Loading

@PVince81 Adjusted. Ready for another round

PVince81
PVince81 reviewed Jan 30, 2020
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

See further comments, it's better but still missing some clarity

@LukasHirt LukasHirt force-pushed the feature/themeable-sharing-indicators branch from ceabe83 to dd32294 Compare January 30, 2020 13:40
@LukasHirt
Move sharing indicators into own component and add extension point
a57ae75 
Moved to smaller components

Load custom indicators as part of extension

Added click handler

Use parentPath and add docs

Added missing sharesTree getter

Extended docs

Added props
@LukasHirt LukasHirt force-pushed the feature/themeable-sharing-indicators branch from dd32294 to a57ae75 Compare January 30, 2020 13:42
@LukasHirt
Copy link
Collaborator Author

@PVince81 Component and its references renamed.

PVince81
PVince81 approved these changes Jan 30, 2020
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Looks great now, thanks a lot! 👍

@LukasHirt LukasHirt merged commit 8fa39d2 into master Jan 30, 2020
@LukasHirt LukasHirt deleted the feature/themeable-sharing-indicators branch January 30, 2020 15:11
@LukasHirt LukasHirt mentioned this pull request Feb 5, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PVince81 PVince81 PVince81 approved these changes

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

Assignees

@LukasHirt LukasHirt

Labels
Category:Enhancement Add new functionality Status:Needs-Review Needs review from a maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extension point sharing indicators for complex theming
2 participants
@LukasHirt @PVince81