Use target endpoints for Service Catalog list and item #544

gosiexon-zen · 2024-11-15T11:44:37Z

Description

Removed mock and call to Custom Objects API and instead use target endpoints created for Service Catalog:

/api/v2/help_center/service_catalog/items.json
/api/v2/help_center/service_catalog/items/{id}.json

For EAP we are reading the service item id from the URL as we are passing it by clicking the item on the list and redirecting. to the service item page. For GA we will read it from curlybars.
Added component for description for service item to handle showing the "read more" button when the description is shorter.
Jira issue GG-3951

Screenshots

Checklist

📗 all commit messages follow the conventional commits standard
⬅️ changes are compatible with RTL direction
♿ Changes to the UI are tested for accessibility and compliant with WCAG 2.1.
📝 changes are tested in Chrome, Firefox, Safari and Edge
📱 changes are responsive and tested in mobile
👍 PR is approved by @zendesk/vikings

sergioazevedo · 2024-11-18T10:23:39Z

src/modules/service-catalog/ServiceCatalogItemPage.tsx

+        const data = await response.json();
+        if (response.ok) {
+          setServiceCatalogItem(data.service_catalog_item);
+        }


Should we move const data = .... to the if block?

Suggested change

const data = await response.json();

if (response.ok) {

setServiceCatalogItem(data.service_catalog_item);

}

if (response.ok) {

const data = await response.json();

setServiceCatalogItem(data.service_catalog_item);

}

Yes we can do this, I moved it.

sergioazevedo · 2024-11-18T10:24:38Z

src/modules/service-catalog/ServiceCatalogItemPage.tsx

+        if (response.ok) {
+          setServiceCatalogItem(data.service_catalog_item);
+        }


I also notice that we are not handling the case where the request is not successful. Shouldn't we at least log the issue?

I don't see this pattern in other places but we definitely will introduce some proper error handling with logging.

luis-almeida · 2024-11-18T14:56:29Z

src/modules/service-catalog/ServiceCatalogItemPage.tsx

  baseLocale,
  hasAtMentions,
  userRole,
  organizations,
  userId,
  brandId,
 }: ServiceCatalogItemPageProps) {
-  const [isExpanded, setIsExpanded] = useState<boolean>(false);
+  const [serviceCatalogItem, setServiceCatalogItem] =
+    useState<ServiceCatalogItem>({} as ServiceCatalogItem);


Could it be set to null or undefined instead of {}? e.g.:

useState<ServiceCatalogItem | undefined>();

Yes, I changed it

luis-almeida · 2024-11-18T14:57:47Z

src/modules/service-catalog/ServiceCatalogItemPage.tsx

-  const toggleDescription = () => {
-    setIsExpanded(!isExpanded);
-  };
+  useEffect(() => {


Could be nice to extract this into a useServiceCatalogItem hook to streamline this component a bit.

Ok I moved it to hook, also we can later move all of the hooks to the hooks folder as we were talking

luis-almeida · 2024-11-18T15:03:57Z

src/modules/service-catalog/components/service-catalog-item/CollapsibleDescription.tsx

+        setShowToggleButton(false);
+      }
+    }
+  }, [description]);


Would it be possible to do handle the toggle based on the length of the description instead of the height?

I changed it to length but I'm not sure if the DESCRIPTION_LENGTH_THRESHOLD = 250 is ok

We can revise it with design later with design 👍🏼

It seems that we need a higher value. I tried a description with 254 characters and it takes 3 rows at the max width:

In this case, we show the "Read more" button but there is no more content to show

But it might differ, letters have different width so it might be more characters but with letters that are thin or the other way around. So I don't think description length based on characters count is not good.

Yeah I know, this is why I think we should set a higher value, or eventually go back to the previous approach.

If we set a higher value, let's say for example 350 characters, it may happen that we will have like 5 rows and we don't show the collapsible button. I don't think it is a big deal if we don't always show exactly 3 lines.
The important thing is that we find a good enough threshold to avoid showing the button when there is no more content to show, and this can be tricky especially considering that we will going to support other languages with different characters sets in the future.

On the other hand, the previous approach is more complex, but if it is the only reliable one maybe we should use it. wdyt @luis-almeida?

We're not using a monospace font so I don't think it's an issue that the rows we end up with might differ a bit.
It's considerably simpler to do the check based on the based on the description.length so I'd go with that approach. We could also just have it in a one-liner without the need for state and effect as mentioned in the other comment 👍🏼

For me safe length is about 270 characters, 350 is too much as from what I've tested. We could also change styling and remove caping it to 3 lines but have some fixed height of the container for description, what is overflowing will be hidden and we will show a button.

We can try 270 👍🏼 I think this is something we can always change entirely if it doesn't work.
But I think still worth it to try start with the simpler approach.

luis-almeida · 2024-11-18T15:04:53Z

src/modules/service-catalog/components/useItemFormFields.tsx

@@ -90,12 +90,14 @@ export function useItemFormFields(

  useEffect(() => {
    const fetchAndSetFields = async () => {


Same here: could be nice to extract this into a useTicketFields hook.

Isn't it too much hooks? Because in this case useItemFormFields will only call another hook and pass fields and onChange method

It's away to abstract the data loading part so the component can be a bit more simple and just hold the rendering logic. But it's fine to leave it here as well.

I will leave it as it is not rendering anything but returning data to the component.

luis-almeida · 2024-11-18T15:10:32Z

templates/service_page.hbs

@@ -14,13 +14,18 @@
  const settings = {{json settings}};
  const container = document.getElementById("service-catalog-item");

+  // Extract the ID from the URL
+  const url = window.location.href;
+  const id = url.substring(url.lastIndexOf('/') + 1);


window.location.href will return query params as well if there's ever some and that might break this logic.
A more resilient way would be window.location.pathname.split("/").pop().
We might as well consider exposing it via curlybars at some point.

Ok I changed that, yes it is only for EAP as I wrote in the description

luis-almeida · 2024-11-18T15:11:00Z

templates/service_page.hbs

+  // Extract the ID from the URL
+  const url = window.location.href;
+  const id = url.substring(url.lastIndexOf('/') + 1);
+
  const props = {
    baseLocale: {{json help_center.base_locale}},
    hasAtMentions: {{json help_center.at_mentions_enabled}},
    userRole: {{json user.role}},
    userId: {{json user.id}},
    brandId: {{json brand.id}},
    organizations: {{json user.organizations}},


Nit as it's not related to this PR but we might not be using all these properties in the app.

When we will be sure what type of fields we will have we can revise this.

gosiexon-zen · 2024-11-19T14:01:43Z

templates/header.hbs

@@ -85,6 +85,7 @@
        {{/if}}
        <li class="item">{{link 'community' role="menuitem"}}</li>
        <li class="item">{{link 'new_request' role="menuitem" class='submit-a-request'}}</li>
+        <li class="item"><a href="/hc/en-us/services">Request a service</a></li>


I added this as I saw we are missing a button in mobile menu

sergioazevedo · 2024-11-21T08:51:40Z

src/modules/service-catalog/useServiceCatalogItem.tsx

+      try {
+        const response = await fetch(
+          `/api/v2/help_center/service_catalog/items/${serviceItemId}`
+        );
+        const data = await response.json();
+        setServiceCatalogItem(data.service_catalog_item);
+      } catch (error) {
+        console.error("Error fetching service catalog item:", error);


Unless I'm missing something here, looks like we are covering only the request happy path, ie when fetch returns a 200 response. What would happen if we get 404 or 503?

Shouldn't we check for response.ok or !response.ok and provide some error handling?

@luis-almeida

That's true I missed that. Can we check response.ok and throw an error so it falls in catch?

Thanks, I changed it to your suggestion.

sergioazevedo · 2024-11-21T08:52:44Z

src/modules/service-catalog/ServiceCatalogList.tsx

+            ? `/api/v2/help_center/service_catalog/items?page[size]=16&${currentCursor}`
+            : `/api/v2/help_center/service_catalog/items?page[size]=16`


I notice that the page[size] parameter is hardcoded to 16 is that intended?

I changed it to some const variable so we can change it easily.

Fredx87 · 2024-11-21T09:54:48Z

src/modules/service-catalog/components/service-catalog-item/CollapsibleDescription.tsx

+        setShowToggleButton(false);
+      }
+    }
+  }, [description]);


It seems that we need a higher value. I tried a description with 254 characters and it takes 3 rows at the max width:

In this case, we show the "Read more" button but there is no more content to show

Fredx87 · 2024-11-21T09:56:45Z

src/modules/service-catalog/components/service-catalog-item/CollapsibleDescription.tsx

+  useEffect(() => {
+    if (description.length > DESCRIPTION_LENGTH_THRESHOLD) {
+      setShowToggleButton(true);
+    } else {
+      setShowToggleButton(false);
+    }
+  }, [description]);


I don't think we need this complex flow with useState and useEffect. We could just do const showToggleButton = description.length > DESCRIPTION_LENGTH_THRESHOLD

Fredx87 · 2024-11-21T09:57:35Z

src/modules/service-catalog/components/service-catalog-item/CollapsibleDescription.tsx

+  const [isExpanded, setIsExpanded] = useState<boolean>(false);
+  const [showToggleButton, setShowToggleButton] = useState<boolean>(false);
+  const { t } = useTranslation();
+  const DESCRIPTION_LENGTH_THRESHOLD = 250;


Nit: this constant can be moved outside the component

Ok I moved it

Fredx87 · 2024-11-21T09:58:33Z

src/modules/service-catalog/useServiceCatalogItem.tsx

+  if (serviceCatalogItem) {
+    return serviceCatalogItem;
+  } else return undefined;


why do we need this? Cannot we just return serviceCatalogItem;?

yes we can 😄 I changed that

Fredx87 · 2024-11-22T13:17:10Z

src/modules/service-catalog/ServiceCatalogItemPage.tsx

+      {serviceCatalogItem && (
+        <>


Do we always need to render the container and then render the content with an empty fragment (<>) as a wrapper? Can we do return serviceCatalogItem ? <Container>...</Container> : null or do we need the container for some reason?

Yes we can do this, I changed that.

Fredx87 · 2024-11-22T13:24:31Z

src/modules/service-catalog/components/service-catalog-item/CollapsibleDescription.tsx

+  return (
+    <DescriptionWrapper>
+      <ItemTitle tag="h1">{title}</ItemTitle>
+      <CollapsibleText expanded={isExpanded}>{description}</CollapsibleText>


Should we set expanded={isExpanded || !showToggleButton}, so we avoid clamping the height to 3 lines if we don't show the toggle button?

Yep, maybe it will help with some edge cases, I added this.

gosiexon-zen requested a review from a team as a code owner November 15, 2024 11:44

gosiexon-zen force-pushed the mbien/use-target-endpoint-for-sc branch from 3c88d26 to 0136e2f Compare November 15, 2024 11:45

gosiexon-zen changed the title ~~Use target endpoint for Service Catalog~~ Use target endpoints for Service Catalog list and item Nov 15, 2024

sergioazevedo reviewed Nov 18, 2024

View reviewed changes

sergioazevedo requested a review from luis-almeida November 18, 2024 10:25

gosiexon-zen requested a review from sergioazevedo November 18, 2024 12:16

luis-almeida reviewed Nov 18, 2024

View reviewed changes

gosiexon-zen force-pushed the mbien/use-target-endpoint-for-sc branch from 0136e2f to 0c8c482 Compare November 19, 2024 13:59

gosiexon-zen commented Nov 19, 2024

View reviewed changes

gosiexon-zen requested a review from luis-almeida November 19, 2024 14:01

sergioazevedo reviewed Nov 21, 2024

View reviewed changes

Fredx87 reviewed Nov 21, 2024

View reviewed changes

gosiexon-zen force-pushed the mbien/use-target-endpoint-for-sc branch 2 times, most recently from 29a3433 to 3a4bd0b Compare November 22, 2024 13:02

Fredx87 reviewed Nov 22, 2024

View reviewed changes

feat: use endpoints for service catalog

36320e2

gosiexon-zen force-pushed the mbien/use-target-endpoint-for-sc branch from 3a4bd0b to 36320e2 Compare November 22, 2024 13:57

Fredx87 approved these changes Nov 22, 2024

View reviewed changes

gosiexon-zen merged commit 2a751d5 into service-catalog-eap Nov 22, 2024
5 checks passed

gosiexon-zen deleted the mbien/use-target-endpoint-for-sc branch November 22, 2024 14:21

		@@ -90,12 +90,14 @@ export function useItemFormFields(

		useEffect(() => {
		const fetchAndSetFields = async () => {

		? `/api/v2/help_center/service_catalog/items?page[size]=16&${currentCursor}`
		: `/api/v2/help_center/service_catalog/items?page[size]=16`

Use target endpoints for Service Catalog list and item #544

Use target endpoints for Service Catalog list and item #544

Conversation

gosiexon-zen commented Nov 15, 2024

Description

Screenshots

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergioazevedo Nov 18, 2024 • edited Loading

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

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

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

sergioazevedo Nov 21, 2024 • edited Loading

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

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

sergioazevedo Nov 18, 2024 •

edited

Loading

sergioazevedo Nov 21, 2024 •

edited

Loading