Catalog from wanda #960

arbulu89 · 2022-11-07T13:27:27Z

Description

Load catalog data from wanda. The same functionality as before, except there is no provider selection. The new view is available through http://localhost:4000/catalog_new

PD: I have included a pair of dummy checks to show the groups in the result

How was this tested?

Tests on jest added

dottorblaster

Nice! Some tweaks and I think we can merge this 👍

assets/js/components/ChecksCatalog/ChecksCatalogNew.jsx

dottorblaster · 2022-11-07T16:34:14Z

assets/js/components/ChecksCatalog/CatalogContainer.jsx

+          <div
+            key={idx}
+            className="check-group bg-white shadow overflow-hidden sm:rounded-md mb-8"
+          >
+            <div className="bg-white px-4 py-5 border-b border-gray-200 sm:px-6">
+              <h3 className="text-lg leading-6 font-medium text-gray-900">
+                {group}
+              </h3>
+            </div>
+            <ul role="list" className="divide-y divide-gray-200">
+              {checks.map((check) => (
+                <li key={check.id}>
+                  <Disclosure>
+                    <Disclosure.Button
+                      as="div"
+                      className="flex justify-between w-full cursor-pointer hover:bg-gray-100"
+                    >
+                      <div className="check-row px-4 py-4 sm:px-6">
+                        <div className="flex items-center">
+                          <p className="px-2 inline-flex text-xs leading-5 font-semibold rounded-full bg-green-100 text-green-800">
+                            {check.id}
+                          </p>
+                          {check.premium > 0 && (
+                            <p className="px-2 inline-flex text-xs leading-5 font-semibold rounded-full bg-green-100 text-green-800">
+                              Premium
+                            </p>
+                          )}
+                        </div>
+                        <div className="mt-2 sm:flex sm:justify-between">
+                          <div className="sm:flex">
+                            <ReactMarkdown
+                              className="markdown text-sm"
+                              remarkPlugins={[remarkGfm]}
+                            >
+                              {check.description}
+                            </ReactMarkdown>


Do we wanna extract all this markup in some more granular components?

Do you mean to create our own markdown component with those defaults?
In this case text-sm would be an additional style.

Can we do the next? Let me merge this as it is about this topic, and I will create other PR with a new markdown component and use it all around the code. What do you think?

Not the markdown, I mean the whole HTML body of this component 😅

In a similar way to https://github.com/trento-project/web/blob/main/assets/js/components/ChecksResults/ChecksResults.jsx, we can make this code more readable just having a wrapper component for the divs and the ul and another component for the li

Anyway if you wanna do that in a subsequent PR, feel free.

@dottorblaster CheckItem extracted to an individual component. Going further looks like an overkill to me

assets/js/components/ChecksCatalog/CatalogContainer.jsx

dottorblaster

Sorry, really. Last thing then we can merge 👍

dottorblaster · 2022-11-08T11:23:25Z

assets/js/trento.jsx

 import ChecksCatalog from '@components/ChecksCatalog';
+import { ChecksCatalogNew } from '@components/ChecksCatalog';


Sorry ❤️ last thing 😄 could you group these imports? You can do that just by doing import ChecksCatalog, { ChecksCatalogNew } from...

CDimonaco · 2022-11-08T11:38:13Z

Looks good to me, I'm with @dottorblaster on all the other comments, I think that we could add some data-testid attributes/props on component to better test them even in the future, like we already done other jest tests.

For reference: https://testing-library.com/docs/queries/bytestid/

It's a nice to have btw, @arbulu89 good job!

arbulu89 · 2022-11-08T11:53:31Z

Looks good to me, I'm with @dottorblaster on all the other comments, I think that we could add some data-testid attributes/props on component to better test them even in the future, like we already done other jest tests.

For reference: https://testing-library.com/docs/queries/bytestid/

It's a nice to have btw, @arbulu89 good job!

Hey @CDimonaco ,
I see the point of using the test-ids. In this case, finding the elements was fairly simple and the tests look pretty easy to understand, that's why I didn't include them. But I agree that once you have difficult dom findings etc they are super useful

CDimonaco · 2022-11-08T11:54:16Z

Looks good to me, I'm with @dottorblaster on all the other comments, I think that we could add some data-testid attributes/props on component to better test them even in the future, like we already done other jest tests.
For reference: https://testing-library.com/docs/queries/bytestid/
It's a nice to have btw, @arbulu89 good job!

Hey @CDimonaco , I see the point of using the test-ids. In this case, finding the elements was fairly simple and the tests look pretty easy to understand, that's why I didn't include them. But I agree that once you have difficult dom findings etc they are super useful

Perfect! LGTM

CDimonaco

LGTM

dottorblaster

fabriziosestito · 2022-11-08T12:23:32Z

assets/js/lib/test-utils/factories.js

+export const catalogCheckFactory = Factory.define(() => ({
+  id: faker.datatype.uuid(),
+  name: faker.animal.cat(),
+  group: faker.animal.cat(),


fabriziosestito · 2022-11-08T12:25:00Z

Looks good to me, I'm with @dottorblaster on all the other comments, I think that we could add some data-testid attributes/props on component to better test them even in the future, like we already done other jest tests.
For reference: https://testing-library.com/docs/queries/bytestid/
It's a nice to have btw, @arbulu89 good job!

Hey @CDimonaco , I see the point of using the test-ids. In this case, finding the elements was fairly simple and the tests look pretty easy to understand, that's why I didn't include them. But I agree that once you have difficult dom findings etc they are super useful

Perfect! LGTM

FYI
In the spirit of the guiding principles, it is recommended to use this only after the other queries don't work for your use case. Using data-testid attributes do not resemble how your software is used and should be avoided if possible.

arbulu89 added the enhancement New feature or request label Nov 7, 2022

arbulu89 added 4 commits November 7, 2022 16:44

Duplicate current catalog view in a new route

2744afc

Implement new catalog container

7f08f4b

Add tests to the catalog container

896fe47

Include process as global variable

0d2e52f

arbulu89 force-pushed the catalog-from-wanda branch from eff714e to 0d2e52f Compare November 7, 2022 15:48

arbulu89 requested review from dottorblaster, CDimonaco, fabriziosestito, nelsonkopliku and rtorrero November 7, 2022 15:51

arbulu89 marked this pull request as ready for review November 7, 2022 15:51

dottorblaster requested changes Nov 7, 2022

View reviewed changes

Show error message if the checks catalog is empty

0e62677

arbulu89 requested a review from dottorblaster November 8, 2022 09:00

arbulu89 commented Nov 8, 2022

View reviewed changes

assets/js/components/ChecksCatalog/CatalogContainer.jsx Show resolved Hide resolved

arbulu89 force-pushed the catalog-from-wanda branch 2 times, most recently from 85aab9e to f3271e2 Compare November 8, 2022 09:50

dottorblaster requested changes Nov 8, 2022

View reviewed changes

Extract check item to individual component

470b64c

arbulu89 force-pushed the catalog-from-wanda branch from f3271e2 to 470b64c Compare November 8, 2022 11:50

arbulu89 requested a review from dottorblaster November 8, 2022 11:53

CDimonaco approved these changes Nov 8, 2022

View reviewed changes

dottorblaster approved these changes Nov 8, 2022

View reviewed changes

fabriziosestito reviewed Nov 8, 2022

View reviewed changes

fabriziosestito approved these changes Nov 8, 2022

View reviewed changes

arbulu89 merged commit ff8d820 into main Nov 8, 2022

arbulu89 deleted the catalog-from-wanda branch November 8, 2022 13:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catalog from wanda #960

Catalog from wanda #960

arbulu89 commented Nov 7, 2022 •

edited

Loading

dottorblaster left a comment

dottorblaster Nov 7, 2022

arbulu89 Nov 8, 2022

arbulu89 Nov 8, 2022

dottorblaster Nov 8, 2022

dottorblaster Nov 8, 2022

arbulu89 Nov 8, 2022

dottorblaster left a comment

dottorblaster Nov 8, 2022

CDimonaco commented Nov 8, 2022

arbulu89 commented Nov 8, 2022

CDimonaco commented Nov 8, 2022

CDimonaco left a comment

dottorblaster left a comment

fabriziosestito Nov 8, 2022

fabriziosestito commented Nov 8, 2022

		import ChecksCatalog from '@components/ChecksCatalog';
		import { ChecksCatalogNew } from '@components/ChecksCatalog';

Catalog from wanda #960

Catalog from wanda #960

Conversation

arbulu89 commented Nov 7, 2022 • edited Loading

Description

How was this tested?

dottorblaster 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

dottorblaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CDimonaco commented Nov 8, 2022

arbulu89 commented Nov 8, 2022

CDimonaco commented Nov 8, 2022

CDimonaco left a comment

Choose a reason for hiding this comment

dottorblaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziosestito commented Nov 8, 2022

arbulu89 commented Nov 7, 2022 •

edited

Loading