Skip to content
This repository has been archived by the owner on Jul 9, 2022. It is now read-only.

Add getValidThreadColors #512

Merged
merged 12 commits into from
Aug 10, 2017
Merged

Add getValidThreadColors #512

merged 12 commits into from
Aug 10, 2017

Conversation

AstroCB
Copy link
Contributor

@AstroCB AstroCB commented Aug 10, 2017

Adds a function to get accepted values for changeThreadColor and validation within changeThreadColor to prevent undefined behavior for non-supported colors as per #506.

@Schmavery
Copy link
Owner

Schmavery commented Aug 10, 2017

Thanks for contributing some more changes :)
Is there any reason you're not just exposing the data directly? As in:

module.exports = function(defaultFuncs, api, ctx) {
    return ["#44bec7", "#ffc300", "#fa3c4c", "#d696bb", "#6699cc", ...];
};

I'm also wondering if it wouldn't be more readable if we actually made it a map of the human-readable names exposed by facebook to the colors. The python clone of this api does this. i.e.

{
    MessengerBlue = ''
    Viking = '#44bec7'
    GoldenPoppy = '#ffc300'
    RadicalRed = '#fa3c4c'
    Shocking = '#d696bb
    ...
}

Might end up with more readable code and make it more possible for us to add/adapt/remove colors as time goes on (though I'll admit the names are... strange at best).

Just seems like it would be nicer to write/read
api.threadColors.RadicalRed than api.getValidThreadColors()[4].

@AstroCB
Copy link
Contributor Author

AstroCB commented Aug 10, 2017

Ah, I haven't seen any constant (non-functional) data exposed via the API so I wasn't sure if that would be against the design you guys have set up.

Are those names the official ones exposed by Facebook? I haven't seen them anywhere.

@AstroCB
Copy link
Contributor Author

AstroCB commented Aug 10, 2017

I switched it to a dictionary using those names; the code should definitely be more readable that way.

I also renamed it to validThreadColors before I saw your note on usage, but I can change it to just threadColors if that's more idiomatic.

@bsansouci
Copy link
Collaborator

Seems good to me though I'd change the docs to say that it is an object rather than it returns an object.

DOCS.md Outdated
<a name="validThreadColors"></a>
### api.validThreadColors

Returns a dictionary mapping names of all currently valid thread colors to their hexadecimal values that are accepted by [`api.changeThreadColor`](#changeThreadColor). These colors, listed below, are the ones present in the palette UI used for selecting thread colors on the Messenger client. Due to Facebook backend changes, the thread color can no longer be set to an arbitrary hex value.
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we can move the note about "Due to Facebook backend changes, the thread color can no longer be set to an arbitrary hex value." to the CHANGELOG.md file and put a link there to this PR? (See the other entries for an example)

@Schmavery
Copy link
Owner

I feel like the validity is implied by the fact that we're providing the colors, so just threadColors is probably more concise. Thanks!

@AstroCB
Copy link
Contributor Author

AstroCB commented Aug 10, 2017

Done.

@Schmavery
Copy link
Owner

Seems good, thanks a lot.

@Schmavery Schmavery merged commit baa0eb4 into Schmavery:master Aug 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants