Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign up[WIP] [#1056] Implement new design for data type picker #108
Conversation
vitorbaptista
requested a review
from
akariv
Jun 8, 2017
vitorbaptista
added
the
in progress
label
Jun 8, 2017
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
akariv
Jun 9, 2017
Contributor
|
- The descriptions and autocomplete are only used for generating the
dropdown, so feel free to modify the API to what works best.
- os-types gets automatically pushed to npm given that a proper commit
message syntax is maintained (see log for examples)
- 40 unknown dimensions - I added them to allow mapping of old OS datasets
into the current platform. If we can make them invisible in the UI then it
would be best.
…On Thu, 8 Jun 2017 at 23:29 Vitor Baptista ***@***.***> wrote:
@vitorbaptista <https://github.com/vitorbaptista> requested your review
on: openspending/os-packager#108
<#108> [WIP] [#1056]
Implement new design for data type picker.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#108 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAQMdW2QcO0tq8aj-SuQGzQMvu9VMa9Iks5sCFmWgaJpZM4N0lTC>
.
|
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
vitorbaptista
Jun 9, 2017
Contributor
@akariv So I could then remove the autoComplete() method of os-types. What about the unknown type descriptions? If I were to change the format from the current:
{
'activity:': {
// ...
},
'activity:generic:': {
// ...
},
// ... and so on
}To the current format used in this PR:
[
{
key: 'activity',
types: [
{
key: 'activity::generic',
types: [],
},
// ...
],
},
// ...
]Should I add the 40 unknown dimensions as well? Is this data used somewhere else? Otherwise it feels unnecessary to add these dimensions only to hide them.
|
@akariv So I could then remove the {
'activity:': {
// ...
},
'activity:generic:': {
// ...
},
// ... and so on
}To the current format used in this PR: [
{
key: 'activity',
types: [
{
key: 'activity::generic',
types: [],
},
// ...
],
},
// ...
]Should I add the 40 unknown dimensions as well? Is this data used somewhere else? Otherwise it feels unnecessary to add these dimensions only to hide them. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
Yes, there's no need for the descriptions of the 40 unknown types. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
vitorbaptista
Jun 12, 2017
Contributor
As we have a new design for os-packager, this PR isn't relevant anymore. Closing it unmerged
|
As we have a new design for |
vitorbaptista commentedJun 8, 2017
This is still WIP only because we need to release a new version of
os-types(we need the code merged on openspending/os-types#8). Other than that, it's good to go, although the performance isn't great, as there're many different data types.@akariv Why do we add 40
unknown dimension #ito the data types on https://github.com/openspending/os-types/blob/master/src/index.js#L18-L43 ? As we're using the data to build the options, we end up getting them all. Also, I had to change the structure ofos-type-descriptions.jsonto make it easier to build the menu (seeconvertOSTypesDescriptionsinapp/front/scripts/directives/os-datatype.js). This could be avoided if we're able to change it onos-types. Finally, there might be some methods onos-typesthat aren't needed anymore, likeautoComplete(), but I'm not sure who uses it.Fixes openspending/openspending#1056