Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add displayConfig in Hub Catalog, update GalleryDisplayConfigSchema #1747

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

vivzhang
Copy link
Contributor

@vivzhang vivzhang commented Nov 22, 2024

affects: @esri/hub-common

ISSUES CLOSED: 11582

  1. Description:

  2. Instructions for testing:

  3. Closes Issues: # (if appropriate)

  4. Updated meaningful TSDoc to methods including Parameters and Returns, see Documentation Guide

  5. used semantic commit messages

  6. PR title follows semantic commit format (CRITICAL if the title is not in a semantic format, the release automation will not run!)

  7. updated peerDependencies as needed. CRITICAL our automated release system can not be counted on to update peerDependencies so we must do it manually in our PRs when needed. See the updating peerDependencies section of the release instructions for more details.

Copy link
Contributor

@jordantsanz jordantsanz left a comment

Choose a reason for hiding this comment

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

Super nice work here -- one comment re: showThumbnail, the rest are all minor.

Comment on lines 103 to 108
corners: { type: "string", enum: ["square", "round"], default: "square" },
shadow: {
type: "string",
enum: ["none", "low", "medium", "heavy"],
default: "none",
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have some enums for these in common/core/schemas/shared/enums.ts, i.e. drop shadow, corners, etc

Let's reuse those here / create some new enums in that file for the other properties

Comment on lines 193 to 200
layout?: "list" | "grid" | "map" | "table" | "calendar" | "compact";
cardTitleTag?: "h1" | "h2" | "h3" | "h4" | "h5" | "h6";
showThumbnail?: boolean;
corners?: "square" | "round";
shadow?: "none" | "light" | "medium" | "heavy";
showLinkButton?: boolean;
linkButtonStyle?: "solid" | "outline" | "outline-fill" | "transparent";
linkButtonText?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here about enums, let's reuse those here

Comment on lines +222 to +224
// AGO has a util for determining display name and item type icons
// that we can use for reference
// https://devtopia.esri.com/WebGIS/arcgis-app-components/blob/master/src/components/arcgis-item-type/utils.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

oh wow...should we be using this for the combobox/item type work then?

Copy link
Contributor Author

@vivzhang vivzhang Nov 27, 2024

Choose a reason for hiding this comment

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

I'd say we stick with our getContentTypeIcon for now for consistency. I added this comment here because I just got that info from the AGO team, it was new to me as well. I think this needs a holistic review by Design first before we decide what to do.

Comment on lines 98 to 102
showThumbnail: {
oneOf: [{ type: "string" }, { type: "boolean" }],
enum: [true, false, "grid"],
default: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure the config editor is built to handle oneOf yet in the schema.

Could we:

  1. Make this just type string
  2. Update the enum here to "show"/"grid"/"hide"
  3. Update the default here to "show"

Then in the arcgis-hub-catalog component:

showThumbnail = (showThumbnail == "grid") ? layout == "grid" : showThumbnail;

That way we don't have to deal with booleans and strings at the same time, and in the catalog component we cast everything the way we want it to first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants