Lets you display a list with xml objects in a grid, makes multiple pages if not all the list can be displayed as one.
Details
- Reviewers
- None
- Group Reviewers
Restricted Owners Package (Owns No Changed Paths)
Use case of D1703: Map browser for gamesetup
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Event Timeline
Code structure:
It's particularly hard to judge this thing without any examplary / the only use case, so I would recommend to create one diff for the mapbrowser.
After examination, code can probably be clarified or simplified a bit.
Use case analysis:
I'm wondering if there is a second use case for this prototype or if it's only used to achieve object-oriented style over procedural style.
What are other use cases? A hero selection dialog? A relic selection dialog?
It's not only a Grid but also includes Paging. In comparison to implementing paging in a separate prototype, adding it to the grids prototype gains less advantages than the same code in two prototypes for instance.
Typically a "GUI addon" is usually a type of a GUI object or a property of GUI object type, defined by the GUI in source/gui/.
So perhaps float-left property from css is what would allow GUI objects to be aligned as a grid without adding any JS code and allowing a number of other problems to be fixed simultaneously? (For example aligning the input to the label in: Label: <input>).
I acknowledge that this is difficult to implement, but my impression of the prototype is that it claims to be more generic than it is.
I propose to upload the MapBrowser combined with its dependencies.
binaries/data/mods/public/gui/common/GridBrowser.js | ||
---|---|---|
21 ↗ | (On Diff #6929) | "GUI objects" |
22 ↗ | (On Diff #6929) | indirection making it more complicated than it needs to be |
55 ↗ | (On Diff #6929) | Pass the function to be called as an argument to the constructor or other function. |
58 ↗ | (On Diff #6929) | unnecessary indirection |
73 ↗ | (On Diff #6929) | unnecessary indirection |
77 ↗ | (On Diff #6929) | "" |
90 ↗ | (On Diff #6929) | these two functions are always called together.. |
151 ↗ | (On Diff #6929) | simplify with min/max or modulo? |
169 ↗ | (On Diff #6929) | unneeded parentheses and Math.max(1, numPages) |
174 ↗ | (On Diff #6929) | No need for template strings (never seen them being used in our JS code either) |
177 ↗ | (On Diff #6929) | unnecessary indirection |
binaries/data/mods/public/gui/common/GridBrowser.js | ||
---|---|---|
17 ↗ | (On Diff #6929) | Why are these not in the constructor? |
I'm wondering if there is a second use case for this prototype or if it's only used to achieve object-oriented style over procedural style.
What are other use cases? A hero selection dialog? A relic selection dialog?
A new mod browser for mod.io, now that they have implemented a proper versioning system (https://docs.mod.io/changelog/#how-we-handle-versioning) we can use the API to the fullest without worrying about breaking changes. Including getting the description and screenshots and stuff.
binaries/data/mods/public/gui/common/GridBrowser.js | ||
---|---|---|
17 ↗ | (On Diff #6929) | Not any specific reason. Seems the constructor choice would be better. |
21 ↗ | (On Diff #6929) | So change xml object to gui object ? |
22 ↗ | (On Diff #6929) | I think the variable defines better the meaning of the value. |
55 ↗ | (On Diff #6929) | Correct. Passing the function as argument seems simpler. |
58 ↗ | (On Diff #6929) | I think is having setter function homogenizes the function naming convention. |
73 ↗ | (On Diff #6929) | Same as the _setList function. |
77 ↗ | (On Diff #6929) | correct |
90 ↗ | (On Diff #6929) | What two functions? |
151 ↗ | (On Diff #6929) | Can't see a way to do it. |
174 ↗ | (On Diff #6929) | What is the alternative? |
177 ↗ | (On Diff #6929) | Same as _setList and _setChildDimensions. |
binaries/data/mods/public/gui/common/GridBrowser.js | ||
---|---|---|
174 ↗ | (On Diff #6929) | sprintf. Or just regular concatenation if possible. |
binaries/data/mods/public/gui/common/GridBrowser.js | ||
---|---|---|
174 ↗ | (On Diff #6929) | I changed to concatenation but I still think string templates are the best option given that reduces needless code duplication of sprintf and is much clear than concatenation. Was sprintf was created before spidermonkey had support for string templates? |
binaries/data/mods/public/gui/common/GridBrowser.js | ||
---|---|---|
174 ↗ | (On Diff #6929) | String templates came with ES 2015 IIRC. That file was added in 2014 deducing from a quick file history lookup (probably even earlier). Which raises the question of why its still present? |
binaries/data/mods/public/gui/common/GridBrowser.js | ||
---|---|---|
174 ↗ | (On Diff #6929) | sprintf are used in 0ad for translation, the rest of the code uses implicit string conversion |
I'd say that this part can be better in the C++ part. Because it may be faster and has much more information about elements.
binaries/data/mods/public/gui/common/GridBrowser.js | ||
---|---|---|
36 ↗ | (On Diff #6934) | These makes it harder to follow. Combine them and possibly add an update() method. |
Rewrote lots of superfluous code. Added useful functions and features (animation).
Cleaner code.