Feature 1773 layer categories #2274

yvonnesjy · 2024-02-20T23:17:48Z

Support "layerCategories" as an XML config option that has "layers" nested under an array of category objects. This is so that we can group and collapse layers as the number of supported layers grows.

The "layers" config option is still supported for portals that don't use categories.

Fixes #1773

IconUtilities contains code shared by MapAsset and a new AssetCategory model (in a separate commit).

The two classes are modeled after MapAsset and MapAssets.

yvonnesjy · 2024-02-20T23:25:14Z

src/js/collections/maps/AssetCategories.js

+   * @class AssetCategories
+   * @extends Backbone.Collection
+  //  TODO: yvonneshi - update
+  //  * @since x.x.x


Current release is 2.27.1 and this is targeting end of March release. How do I derive the release number here?

Robyn fills in these x.x.x strings during release time. Safe to leave as x.x.x

src/js/models/maps/AssetCategory.js

ianguerin

Going forward, it's helpful for me to see unit tests alongside implementation. Obviously we're getting acquainted with this codebase and new dev workflow, but I wanted to say that out loud

src/js/models/maps/assets/MapAsset.js

src/js/common/IconUtilities.js

src/js/collections/maps/AssetCategories.js

src/js/models/maps/AssetCategory.js

ianguerin · 2024-02-21T16:08:43Z

docs/screenshots/views/maps/LayerCategoryListView.png

It seems the screenshot is missing some icons?

Some layer icons 404'ed, which is out of scope for this PR. I intentionally left some of the category icons blank to show that's how it looks if portal owner doesn't specify one

src/js/views/maps/LayerCategoryItemView.js

src/js/views/maps/MapView.js

src/js/collections/maps/AssetCategories.js

src/js/templates/maps/layer-category-item.html

robyngit · 2024-02-22T21:23:44Z

Hey @yvonnesjy looks like you made a ton of progress super quickly, nice! 🎉 I saw you said that this PR is a WIP so I set it as a "draft" for now. Please click "ready for review" and assign me as a reviewer when you feel it's ready.

Maybe it's something your working on, but I did a little testing and just wanted to make sure you're aware that the changes here do seem to break existing cesium map set ups. For example, if you switch the the theme to DRP and look at the data page (which uses the same Map model and CesiumWidgetView), the map doesn't render because of an error with geohashes:

DRP error details

Uncaught TypeError: geohashLayer.replaceGeohashes is not a function
    at n.updateGeohashCounts (Map-Search.js?v=2.27.1:302:22)
    at n.<anonymous> (Map-Search.js?v=2.27.1:177:16)
    at _ (backbone.js:370:57)
    at m (backbone.js:356:19)
    at f (backbone.js:155:16)
    at u.trigger (backbone.js:346:5)
    at n.reset (backbone.js:952:33)
    at t.success (backbone.js:1050:20)
    at c (jquery-1.9.1.min.js?v=2.27.1:3:7857)
    at Object.fireWith [as resolveWith] (jquery-1.9.1.min.js?v=2.27.1:3:8658)

I don't want to expand your work to entail anything outside of the PDG, but one requirement is implementing layer categories is that the changes don't break existing portals or data catalogs that are using cesium.

I won't go too deep into reviewing while you're still working on things, please just let me know if/when you'd like feedback.

yvonnesjy · 2024-02-23T08:59:03Z

@robyngit Nice catch and I'm happy to report that the unit tests caught the bug too! This should be fixed now and the PR is ready for review! (I don't have access to assign reviewers)

BTW can you elaborate on "switch the theme to DRP and look at the data page"? Should be handy for future changes

yvonnesjy · 2024-02-23T09:21:54Z

src/js/models/maps/Map.js

-       * MapAssetConfigs are passed to a {@link MapAssets} collection.
+       * MapAssetConfigs are passed to a {@link MapAssets} collection. When layerCategories
+       * exist, this property will be ignored.
+       * @property {MapConfig#MapAssetConfig[]} [layerCategories] - A collection of


@robyngit Given the new field introduced here, I imagine we need an update to docs/docs/MapConfig.html? Blame suggests these docs are autogenerated before release though- I take that it's a noop in this PR?

Yes! Thank you for thinking of documentation. :) All of the API docs are auto-generated from the JSdocs in all of the JS files. That's why it's so important to add/update JSdocs for any new or changed methods or properties. These docs are built at each release. If you'd like to test the docs your self, see this README. (But please don't commit the changes from your new doc build, it just complicates the git & PR history.)

It looks like you added the new option to the MapConfig namespace so the matching documentation will be generated at release time! 🎉

The two views are modeled after LayerListView and LayerItemView. The styling of the views doesn't fully represent the mock yet. Specifically: - Margin of layer items - Padding of tool bar panel - "Layers" title - Font and color of icons and texts These will be done in separate PRs. Mock: https://www.sketch.com/s/eea6a4f3-5c59-40e6-bf7f-64edc511e4cb

1. Add layerCategories as an attribute which takes its value from the XML config. 2. Add getLayerGroups() that supports both layers and layerCategories, while layerCategories takes precedence. 3. Replace all call sites of map.get("layers") with map.getLayerGroups().

The try-catch blocks appear throughout the codebase but don't meaningfully handle the the errors. I went through the changes in this PR and removed all instances of try-catch where we simply console.log the error.

Thanks to unit tests!

Sinon seems to be the standard framework that works with Mocha and chai. See sinonjs.org

1. Add a default layer category icon (same icon as the layers button on the sidebar) 2. Update expand/collapse toggle to use caret instead of chevron

mbjones · 2024-02-28T00:25:22Z

@yvonnesjy Robyn has been out, so I will add my minimally-informed 2 cents, in hope it might help you move forward.

Set a theme by changing the theme path in the root index.html file

// The path to your configuration file for MetacatUI. This can be any web-accessible location.
var appConfigPath = "/js/themes/drp/config.js";

Themes we maintain are either 1) in the metacatui repo itself (see https://github.com/NCEAS/metacatui/tree/main/src/js/themes), or 2) in https://github.com/NCEAS/metacatui-themes (which is where the drp theme is located). In reality themes can come from anywhere as they are just a collection of html, css, and js template files. Some sites maintain their own.

@iannesbitt can probably help with the drp testing, as he maintains that theme.

robyngit · 2024-02-28T19:16:47Z

@yvonnesjy

Nice catch and I'm happy to report that the unit tests caught the bug too! This should be fixed now and the PR is ready for review! (I don't have access to assign reviewers)

Yay for unit tests! :) Nice work

BTW can you elaborate on "switch the theme to DRP and look at the data page"? Should be handy for future changes

Oops! Sorry for glossing over that without any instructions. DRP is one of our themes like "arctic", "knb", or "dataone", except that it is not shipped with MetacatUI; it's an external theme. External themes are stored in the metacatui-themes repository. So to test MetacatUI with the DRP theme, you need to clone the themes repo, then symlink the drp theme to the MetacatUI themes directory:

ln -s {PATH-TO-METACATUI-THEMES-REPO}/src/drp/js/themes/drp {PATH-TO-METACATUI-REPO}/src/js/themes/drp

You can set your appConfig path like Matt indicated, OR in your config/config.js file, you can simply set the "theme" to drp.

Let me know if you have any more questions about this or if I can clarify anything at all!

Testing in DRP revealed a bug that should've been caught by unit tests. Added more test coverage to files affected by refactoring.

src/js/views/maps/CesiumWidgetView.js

yvonnesjy · 2024-03-01T10:54:30Z

src/js/models/maps/assets/MapAsset.js

@@ -719,101 +731,10 @@ define([
       * @param {string} icon An SVG string to use for the MapAsset icon
       */
      updateIcon: function (icon) {


An error here is handled in initialize:model.set("iconStatus", "error")

yvonnesjy · 2024-03-01T10:57:05Z

src/js/views/maps/MapView.js

@@ -268,36 +268,30 @@ define(
         * @returns {LayerDetailsView} Returns the rendered view
         */
        renderLayerDetails: function () {


An error here is silently caught in render()

src/js/models/connectors/Map-Search.js

robyngit

Awesome @yvonnesjy! Really appreciate how thorough you are with the tests. Only very minor comments (in line). The only other suggestion is that we should handle the situation gracefully when there are no layers and no layerCategories configured. Given that the portals will be configurable by users, this could theoretically happen. If you set both config.layers and config.layerCategories to null, you'll see the entire portal fails to render (beyond just the map). Ideally, we should see a blank Cesium globe and the portal in this case. Very excited to see this in production! 🎉

src/js/collections/maps/AssetCategories.js

robyngit · 2024-03-05T17:02:23Z

src/js/collections/maps/AssetCategories.js

+    /** @lends AssetCategories.prototype */ {
+
+      model: AssetCategory,
+      modelId: attrs => {


Where is this modelId used?

This one I truly forget and can't seem to reproduce an error without it. Removed and tests are still passing.

robyngit · 2024-03-05T17:02:36Z

src/js/collections/maps/MapAssets.js

@@ -98,6 +98,8 @@ define([
        }
      },

+      modelId: attrs => attrs.cid,


Same for this modelId, where is it used?

Oh my- here's the mental roller coaster I went through just for the giggles: The need for modelId is so tucked away until something (deep inside backbone) breaks very subtly. Multiple times now I added this fix and then removed it later cause I forgot why... until I went down the rabbit hole again... but I didn't learn my lesson to add a comment right away!

Now I've just gotten getting so upset cause I forgot the reason once again... until I realized the new tests caught it!!

That said, here's the issue, and what a better fix I've found since:

Problem: Without explicit modelId, backbone picks up the default "" as id for every map asset (unless they're set in XML, which isn't the case for PDG). This creates a problem whenever MapAssets.add/remove is called, or event listeners are involved. Specifically here, where this.get(model) references id, which is "" for every model.

Fix: After spending more time on this, I think rather than adding modelId which I keep forgetting the reasons for, a better fix would just be to remove id: "" from the default.

Oh no, what a frustrating sounding issue!! It seems like having "id" as a model attribute in MapAsset is a conflict with backbone. This attribute is intended to be used to specify the package ID for an associated dataset on a dataone repo (like the Arctic Data Center). I think we could rename this pid (what the IDs are usually called on dataone), or maybe datapackage_id. This will mean updating the PDG and DRP portal xml, but I think it's better we do this now while cesium isn't in wide use and we have control over the portal document content.

robyngit · 2024-03-05T17:03:19Z

src/js/common/IconUtilities.js

@@ -0,0 +1,74 @@
+/*global define */


Thank you for moving this to its own module! 👍🏻

src/js/models/connectors/Map-Search.js

robyngit · 2024-03-05T18:31:24Z

src/js/views/maps/LayerCategoryItemView.js

+         */
+        template: _.template(Template),
+
+        classNames: {


I think it would be helpful to include JS docs here indicating on which elements each class is used

hmmm what do you mean by "elements"? as in the html tags?

More generally, what parts of the view those html tags represent or what the class is used for.

src/js/views/maps/LayerCategoryItemView.js

src/js/views/maps/LayerCategoryListView.js

src/js/views/maps/CesiumWidgetView.js

yvonnesjy · 2024-03-06T06:49:20Z

Awesome @yvonnesjy! Really appreciate how thorough you are with the tests. Only very minor comments (in line). The only other suggestion is that we should handle the situation gracefully when there are no layers and no layerCategories configured. Given that the portals will be configurable by users, this could theoretically happen. If you set both config.layers and config.layerCategories to null, you'll see the entire portal fails to render (beyond just the map). Ideally, we should see a blank Cesium globe and the portal in this case. Very excited to see this in production! 🎉

weeeee. fixed the issue by returning [] instead of null in getLayerGroups()

Logic changes: 1. Removed default "" id from MapAsset so that Backbone doesn't confuse all MapAsset's as one 2. Stop listening before adding listeners in Map-Search.js 3. In initialize()'s, check for existence of options before dereferencing

I couldn't bring myself to write JSDoc for these classnames that are supposed to be implementation details. We'll keep them in the "private" space as file constants instead.

robyngit

🎉🎉🎉

yvonnesjy added 2 commits February 19, 2024 22:41

Layer Categories: Create IconUtilities

9be9485

IconUtilities contains code shared by MapAsset and a new AssetCategory model (in a separate commit).

Layer Categories: Create AssetCategory model and collection

4de2adc

The two classes are modeled after MapAsset and MapAssets.

yvonnesjy commented Feb 20, 2024

View reviewed changes

robyngit marked this pull request as draft February 21, 2024 15:02

robyngit linked an issue Feb 21, 2024 that may be closed by this pull request

Support nested data layers #1773

Closed

ianguerin reviewed Feb 21, 2024

View reviewed changes

ianguerin approved these changes Feb 22, 2024

View reviewed changes

src/js/collections/maps/AssetCategories.js Show resolved Hide resolved

ianguerin reviewed Feb 22, 2024

View reviewed changes

src/js/templates/maps/layer-category-item.html Outdated Show resolved Hide resolved

yvonnesjy requested a review from ianguerin February 23, 2024 09:04

yvonnesjy marked this pull request as ready for review February 23, 2024 09:05

yvonnesjy commented Feb 23, 2024

View reviewed changes

yvonnesjy added 6 commits February 23, 2024 02:55

Layer Categories: Add LayerCategoryList to tool bar

e0d6126

Layer Categories: Remove try-catch blocks + readability fixes

d5c25ba

The try-catch blocks appear throughout the codebase but don't meaningfully handle the the errors. I went through the changes in this PR and removed all instances of try-catch where we simply console.log the error.

Layer Categories: Bug fixes and small improvements

ca53a94

Thanks to unit tests!

Layer Categories: Install Sinon for spy, stub, and mock

e1ecbbe

Sinon seems to be the standard framework that works with Mocha and chai. See sinonjs.org

yvonnesjy force-pushed the feature-1773-layer-categories branch 2 times, most recently from 36e3d39 to 6ed8091 Compare February 25, 2024 02:11

yvonnesjy added 2 commits February 26, 2024 19:23

Layer Categories: Add unit tests

6834263

Layer Categories: UI updates based on new mocks

57ea8c7

1. Add a default layer category icon (same icon as the layers button on the sidebar) 2. Update expand/collapse toggle to use caret instead of chevron

yvonnesjy force-pushed the feature-1773-layer-categories branch from 6ed8091 to 57ea8c7 Compare February 27, 2024 03:23

ianguerin approved these changes Feb 27, 2024

View reviewed changes

yvonnesjy requested a review from robyngit February 27, 2024 23:53

yvonnesjy added 2 commits March 3, 2024 18:14

Layer Categories: Bug fixes and more tests

1caa6c2

Testing in DRP revealed a bug that should've been caught by unit tests. Added more test coverage to files affected by refactoring.

Layer Categories: Error handling improvements

c01edc1

yvonnesjy commented Mar 4, 2024

View reviewed changes

yvonnesjy mentioned this pull request Mar 5, 2024

Properly handle errors in Map-Search.js #2288

Open

robyngit requested changes Mar 5, 2024

View reviewed changes

Layer Categories: Address Robyn's comments

d1bcbf1

Logic changes: 1. Removed default "" id from MapAsset so that Backbone doesn't confuse all MapAsset's as one 2. Stop listening before adding listeners in Map-Search.js 3. In initialize()'s, check for existence of options before dereferencing

ianguerin mentioned this pull request Mar 7, 2024

Add sinon module for use in unit test spying #2293

Closed

Layer Categories: Move classnames variable out of view class

806c15e

I couldn't bring myself to write JSDoc for these classnames that are supposed to be implementation details. We'll keep them in the "private" space as file constants instead.

robyngit approved these changes Mar 13, 2024

View reviewed changes

robyngit merged commit c376a11 into NCEAS:develop Mar 13, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature 1773 layer categories #2274

Feature 1773 layer categories #2274

yvonnesjy commented Feb 20, 2024 •

edited

Loading

yvonnesjy Feb 20, 2024

ianguerin Feb 21, 2024

yvonnesjy Feb 21, 2024

ianguerin left a comment

ianguerin Feb 21, 2024

yvonnesjy Feb 21, 2024

robyngit commented Feb 22, 2024

yvonnesjy commented Feb 23, 2024 •

edited

Loading

yvonnesjy Feb 23, 2024

robyngit Feb 28, 2024

mbjones commented Feb 28, 2024

robyngit commented Feb 28, 2024

yvonnesjy Mar 1, 2024

yvonnesjy Mar 1, 2024

robyngit left a comment

robyngit Mar 5, 2024

yvonnesjy Mar 6, 2024

robyngit Mar 5, 2024

yvonnesjy Mar 6, 2024

robyngit Mar 7, 2024

robyngit Mar 5, 2024

robyngit Mar 5, 2024

yvonnesjy Mar 6, 2024

robyngit Mar 7, 2024

yvonnesjy commented Mar 6, 2024

robyngit left a comment

Feature 1773 layer categories #2274

Feature 1773 layer categories #2274

Conversation

yvonnesjy commented Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianguerin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robyngit commented Feb 22, 2024

yvonnesjy commented Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbjones commented Feb 28, 2024

robyngit commented Feb 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robyngit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yvonnesjy commented Mar 6, 2024

robyngit left a comment

Choose a reason for hiding this comment

yvonnesjy commented Feb 20, 2024 •

edited

Loading

yvonnesjy commented Feb 23, 2024 •

edited

Loading