-
Notifications
You must be signed in to change notification settings - Fork 13
Edit Stream Schema #39
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
Smarker
commented
Feb 28, 2018
- Fixes any incorrect pipelinekeys, streamfactories
- Only shows one stream factory - facebook and instagram have two
| Facebook: { | ||
| pipelineKey: 'Facebook', | ||
| pipelineLabel: 'Facebook', | ||
| pipelineIcon: 'Facebook Icon', |
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.
Shouldn't these be font-awesome class-names? Like fa fa-facebook-official or fa fa-twitter (the latter we use in some of the seed data).
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'll look into that
|
I was only able to take a quick look here, but I'd like to note that the concept of a Stream Factory should not be exposed to the user (not sure if it is in this PR, but I'm mentioning it just in case). The correct factory should be chosen based on the settings provided by the user. In some cases, this may be implemented with a drop-down to select an identifier which is one-to-one mapped with an underlying stream factory, but this should not be strictly true. |
|
yes I agree, I didn't label it as stream factory, but it's good to note. from your comment, when you said settings provided by the user I thought of an idea. Rather than having them enter in the factory, we can just interpret the factory based on the fields they enter. (that way there's no need to have another select option the user needs to fill out) e.g. for Instagram: |
|
Perfect! |