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

feat: handle social links with LinkInput widget #5000

Merged
merged 3 commits into from
Sep 9, 2020
Merged

feat: handle social links with LinkInput widget #5000

merged 3 commits into from
Sep 9, 2020

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented Sep 7, 2020 •
edited
Loading

Fixes #4609

captured-social

@vercel vercel bot temporarily deployed to Preview September 7, 2020 15:37 Inactive
@vercel
Copy link

vercel bot commented Sep 7, 2020 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/eventyay/open-event-frontend/1qbj6hbu7
✅ Preview: https://open-event-frontend-git-fork-snitin315-feat-social-links.eventyay.vercel.app

@auto-label auto-label bot added the feature label Sep 7, 2020
iamareebjamal
iamareebjamal reviewed Sep 7, 2020
View reviewed changes
app/utils/computed-helpers.js Outdated
@@ -14,19 +14,33 @@ export const computedSegmentedLink = function(property) {
return computed(property, {
get() {
const splitted = this.get(property) ? this.get(property).split('://') : [];
const socialPlatforms = ['twitter', 'facebook', 'instagram', 'linkedin', 'youtube', 'github', 'gitlab', 'vimeo', 'flicker'];
Copy link
Member

Choose a reason for hiding this comment

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

Move this up and export this and import it in the component above to remove duplication

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@codecov
Copy link

codecov bot commented Sep 7, 2020 •
edited
Loading

Codecov Report

Merging #5000 into development will decrease coverage by 0.01%.
The diff coverage is 15.78%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #5000      +/-   ##
===============================================
- Coverage        22.69%   22.67%   -0.02%     
===============================================
  Files              485      485              
  Lines             5165     5177      +12     
  Branches            21       21              
===============================================
+ Hits              1172     1174       +2     
- Misses            3989     3999      +10     
  Partials             4        4
Impacted Files Coverage Δ
...omponents/forms/admin/content/social-links-form.js 0.00% <ø> (ø)
app/components/forms/session-speaker-form.js 0.00% <ø> (ø)
app/components/forms/wizard/other-details-step.js 0.00% <ø> (ø)
app/mixins/event-wizard.js 1.05% <0.00%> (-0.03%) ⬇️
app/utils/computed-helpers.js 21.73% <14.28%> (-0.77%) ⬇️
app/components/widgets/forms/link-input.js 32.14% <20.00%> (-9.97%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d18a5c4...4d28976. Read the comment docs.

@iamareebjamal
Copy link
Member

Have you checked that it doesn't interfere with any other place it's being used?

@snitin315
Copy link
Member Author

Let me implement #4609 too in this PR, to make sure we aren't breaking anything.

@snitin315 snitin315 changed the title feat: implement social-links dropdown in other-details step [WIP] feat: implement social-links dropdown in other-details step Sep 7, 2020
@auto-label auto-label bot removed the feature label Sep 7, 2020
@vercel vercel bot temporarily deployed to Preview September 8, 2020 09:15 Inactive
@snitin315 snitin315 changed the title [WIP] feat: implement social-links dropdown in other-details step feat: handle social links with LinkInput widget Sep 8, 2020
@auto-label auto-label bot added the feature label Sep 8, 2020
@mariobehling
Copy link
Member

mariobehling commented Sep 8, 2020 •
edited
Loading

Is it possible to have a custom field also? Right now it is still possible to define the description yourself, e.g. I just added "Add to calendar" on the OSI event.

Screenshot from 2020-09-08 12-21-47
Compare https://eventyay.com/e/8fa7fd14

@snitin315
Copy link
Member Author

snitin315 commented Sep 8, 2020 •
edited
Loading

Is it possible to have a custom field also? Right now it is still possible to define the description yourself, e.g. I just added "Add to calendar" on the OSI event.

@mariobehling Yes, the possible solution I can think of this right now is that we can add one more + button for the custom field.
- Delete + Social Link + Custom Link

@mariobehling
Copy link
Member

There is a way to have a dropdown menu with predefined fields and additionally you can enter a custom name. Google has something like that in their contact manager. Please see below. We need this as well.

Screenshot from 2020-09-08 15-37-42

@snitin315
Copy link
Member Author

We will probably need to write our custom component for this behavior. Can we do it with a separate Issue/PR?

@iamareebjamal
Copy link
Member

It will take more time @mariobehling as there is no such component already in semantic UI.

@mariobehling
Copy link
Member

Ok, thanks for the info.

Yes, the possible solution I can think of this right now is that we can add one more + button for the custom field.

It seems then a custom field would be an immediate option even though it would make the UI a bit more complicated. Could you add this here as well? Basically it would seem we keep the current way and separate it into two components a) social links as covered in the PR already and b) simply using the former way for custom links.

@snitin315
Copy link
Member Author

custom-link

@snitin315
Copy link
Member Author

/cc @iamareebjamal @mariobehling

@mariobehling
Copy link
Member

Are all the link displays working on the public event page?

@snitin315
Copy link
Member Author

Yes.

Screenshot at 2020-09-09 19-17-31

mariobehling
mariobehling approved these changes Sep 9, 2020
View reviewed changes
Copy link
Member

@mariobehling mariobehling left a comment

Choose a reason for hiding this comment

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

Working for tests I did.

@iamareebjamal iamareebjamal merged commit c4fe0de into fossasia:development Sep 9, 2020
@mariobehling
Copy link
Member

The problem is that these changes always have some influence on areas that are not mentioned in the PR. I am no longer able to nullify (delete) admin social links at https://eventyay.com/admin/content, e.g. cannot delete the facebook link.

@mariobehling mariobehling mentioned this pull request Sep 9, 2020
Closed
@snitin315 snitin315 deleted the feat/social-links branch September 15, 2020 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mariobehling mariobehling mariobehling approved these changes

@iamareebjamal iamareebjamal Awaiting requested review from iamareebjamal

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Forms: Social Media - Only request to fill in short form, e.g. username for link specification
3 participants
@snitin315 @iamareebjamal @mariobehling