Closed javagl closed 2 years ago
Thanks @weegeekps for the feedback. I'll address the inlined comments and mark them resolved as soon as they are done.
High level:
return ... that HTML ...
done without a 'manual for
loop (and that was one of the main points that the top-level TODO_GENERALIZATION
in this file referred to). But I was still going a bit back and forth about the structures around filterOptions
and filterValues
, and (even that part was clearer) I didn't know how š . I'll have to read more in the React docs about 'how to do stuff like that', and will try your suggestion in the next pass.yield-all
, I'll have to read a bit more. There probably is a simple solution for that as well.Regarding the latter, I have to admit that when I have the choice between
then I tend to do option 2. Code is written (at most) once, but potentially read dozens of times. It may be possible to solve this particular thing with some magic promise(all(reduce(mapper(y => consume), spread(functor(x => create)), dispatch(receiver()))))
one-liner. But when someone who already is familiar with that library couldn't find a solution in 20 minutes, then I'd rather try to find a simple solution first - even if it involves some old-fashioned for
-loop ;-)
For the formatting, we should consider some config and think about how to handle eslint and prettier. There is some eslintConfig
section in the package.json
- apparently, as part of "react-app" - but I think this might be moved into a config file.
Regarding the most recent changes:
yield-all
thing. No, it's not running in parallel. (I think I came close, but it's possible to write some pretty obscure code here...). Everybody is free to "improve" this. filterOptions
should be a Record
or a Map
. The differences are probably negligible here (and I only used Map
because it is supposed to have a more stable iteration order), and they are doing the same thing, and represent the same data structure, ... but still, they are completely different in terms of usage and syntax (gotta love *Script š ). If there is any preference, or something that could be established as 'the thing that should be used in this project', I'd update this accordingly. The inlined comment has been addressed.
This looks great. Feel free to merge it when you're ready.
An early DRAFT of how some FIRST steps for https://github.com/KhronosGroup/glTF-Project-Explorer/issues/151 could be addressed.
An attempt of a summary what has been done:
The
IProjectInfo
structure was generalized.Originally, it contained several properties:
id
,name
,description
,link
- These had different types, and are so common that that are kept here for nowtask
,license
,type
.language
,inputs
,outputs
,tags
: These had all beenstring[]
arrays, and defined the structure of what a "project" isThe latter have been moved into a
Record<string, string[]>
. The keys are just the former property names. The values are the same that they had in the original structure. There is a bunch of questions that arise from that. Some of them are mentioned in https://github.com/KhronosGroup/glTF-Project-Explorer/issues/151 , and to avoid scattering the discussion, I'd suggest to talk about further details there (and not so much in this PR)The
IFilter
-related classes have been generalizedThere had been several places where the fixed set of
IProjectInfo
properties had been used. For example, in theFilterDimension
, and in large parts of the UI, most importantly in theFilterBar
. This has been updated to work with theIProjectInfo#properties
.Cleanups
Removed unused
export
declarations. Added some comments. Still far too few comments, though...The current state seems to work like before (from a basic test), but it contains some hard-coded parts, and some parts that should be reviewed. There are a few places marked with
TODO_GENERALIZATION
- a quick full-text search should reveal some points that may warrant a closer look. Beyond that, it's mainly a marker for myself, for places that are in a draft state.