feat: bare bones taxonomy detail page [FC-0036] #655

rpenido · 2023-10-24T14:02:44Z

Description

Add the first version of the Taxonomy Detail page to show taxonomy metadata and its tag list.

Supporting Information

Closes [Tagging] "Bare Bones" Taxonomy Detail Page modular-learning#138
Depends on:
- feat: Taxonomy export menu [FC-0036] #645

Testing instructions

Go to the taxonomy list page (http://localhost:2001/taxonomy-list)
Click in any place of the taxonomy card to go to the taxonomy detail page
Verify if the tag list is displayed and the taxonomy info is showed in the right card
Use the Actions menu to export the taxonomy (taxonomy modal implemented here)

If you don't have any taxonomies registered, you could use the taxonomy-sample-data script.

Private-ref: FAL-3529

openedx-webhooks · 2023-10-24T14:02:50Z

Thanks for the pull request, @rpenido! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

e0d · 2023-11-06T15:28:31Z

@rpenido I've invited you to the openedx-triage team which will allow tests to run automatically for your submissions, can you please accept the invitation.

bradenmacdonald · 2023-11-15T19:18:34Z

@rpenido Can you please rebase this and mark it as non-draft, if it's ready? We should also get the 👍🏻 here from your reviewer(s), then schedule the Core Contributor / Maintainer review.

bradenmacdonald · 2023-11-15T19:19:04Z

@rpenido Also, please put [FC-0036] into the PR title.

…l-page

and fix tests to mock useQuery.

pomegranited

👍

I tested this on my devstack
I read through the code
I checked for accessibility issues by navigating using the keyboard only
Includes documentation
User-facing strings are extracted for translation

pomegranited · 2023-11-16T00:56:08Z

@bradenmacdonald Standing in while @rpenido is away: I've merged latest main and addressed the comments from open-craft#5 (review)
I left my 👍 so I think this is ready for upstream review? I don't have permission to remove the "Draft" label.

rpenido · 2023-11-16T01:19:17Z

I don't have permission to remove the "Draft" label.

Done @pomegranited ! Thank you for your cover here! 😃

xitij2000

A few nits.

xitij2000 · 2023-11-17T08:45:04Z

src/taxonomy/tag-list/data/api.js

+    queryFn: () => getAuthenticatedHttpClient().get(getTagListApiUrl(taxonomyId, pageIndex))
+      .then((response) => response.data)
+      .then(camelCaseObject),
+  });


Just realised you can use async/await here:

Suggested change

queryFn: () => getAuthenticatedHttpClient().get(getTagListApiUrl(taxonomyId, pageIndex))

.then((response) => response.data)

.then(camelCaseObject),

});

queryFn: async () => {

const { data } = await getAuthenticatedHttpClient().get(getTagListApiUrl(taxonomyId, pageIndex));

return camelCaseObject(data);

}

});

Addressed with 0e19e99

xitij2000 · 2023-11-17T08:46:51Z

src/taxonomy/tag-list/index.js

@@ -0,0 +1 @@
+export { default as TagListTable } from './TagListTable'; // eslint-disable-line import/prefer-default-export


Just a small nit. Adding it this way means any other entries you add won't need this comment.

Suggested change

export { default as TagListTable } from './TagListTable'; // eslint-disable-line import/prefer-default-export

/* eslint-disable import/prefer-default-export */

export { default as TagListTable } from './TagListTable';

Addressed with 0e19e99

xitij2000 · 2023-11-17T09:08:56Z

src/taxonomy/taxonomy-detail/TaxonomyDetailPage.jsx

+  const renderModals = () => (
+    // eslint-disable-next-line react/jsx-no-useless-fragment
+    <>
+      {isExportModalOpen && (
+        <ExportModal
+          isOpen={isExportModalOpen}
+          onClose={() => setIsExportModalOpen(false)}
+          taxonomyId={taxonomy.id}
+          taxonomyName={taxonomy.name}
+        />
+      )}
+    </>
+  );


Suggested change

const renderModals = () => (

// eslint-disable-next-line react/jsx-no-useless-fragment

<>

{isExportModalOpen && (

<ExportModal

isOpen={isExportModalOpen}

onClose={() => setIsExportModalOpen(false)}

taxonomyId={taxonomy.id}

taxonomyName={taxonomy.name}

/>

)}

</>

);

const renderModals = () => isExportModalOpen && (

<ExportModal

isOpen={isExportModalOpen}

onClose={() => setIsExportModalOpen(false)}

taxonomyId={taxonomy.id}

taxonomyName={taxonomy.name}

/>

);

Addressed with 0e19e99

xitij2000 · 2023-11-17T09:10:26Z

src/taxonomy/taxonomy-detail/TaxonomyDetailPage.jsx

+  const useTaxonomyDetailData = () => {
+    const { isError, isFetched } = useTaxonomyDetailDataStatus(taxonomyId);
+    const taxonomy = useTaxonomyDetailDataResponse(taxonomyId);
+    return { isError, isFetched, taxonomy };
+  };
+
+  const { isError, isFetched, taxonomy } = useTaxonomyDetailData(taxonomyId);


This internal hook doesn't seem to be reused so I'm not sure why it's needed.

Suggested change

const useTaxonomyDetailData = () => {

const { isError, isFetched } = useTaxonomyDetailDataStatus(taxonomyId);

const taxonomy = useTaxonomyDetailDataResponse(taxonomyId);

return { isError, isFetched, taxonomy };

};

const { isError, isFetched, taxonomy } = useTaxonomyDetailData(taxonomyId);

const { isError, isFetched } = useTaxonomyDetailDataStatus(taxonomyId);

const taxonomy = useTaxonomyDetailDataResponse(taxonomyId);

Addressed with 0e19e99

xitij2000 · 2023-11-17T09:13:03Z

src/taxonomy/tag-list/TagListTable.jsx

+  const useTagListData = () => {
+    const { isError, isFetched, isLoading } = useTagListDataStatus(taxonomyId, options);
+    const tagList = useTagListDataResponse(taxonomyId, options);
+    return {
+      isError,
+      isFetched,
+      isLoading,
+      tagList,
+    };
+  };
+
+  const { tagList, isLoading } = useTagListData();


Suggested change

const useTagListData = () => {

const { isError, isFetched, isLoading } = useTagListDataStatus(taxonomyId, options);

const tagList = useTagListDataResponse(taxonomyId, options);

return {

isError,

isFetched,

isLoading,

tagList,

};

};

const { tagList, isLoading } = useTagListData();

const { isLoading } = useTagListDataStatus(taxonomyId, options);

const tagList = useTagListDataResponse(taxonomyId, options);

Addressed with 0e19e99

bradenmacdonald · 2023-11-17T20:26:23Z

@pomegranited Would you be able to address those nits? ^

I also noticed a minor bug with this PR. If I click the "three dots" button, the menu opens - good. But if I click it again while the menu is open, instead of just closing the menu, it also takes me to the taxonomy detail page.

bradenmacdonald · 2023-11-17T20:31:44Z

Also: I think we should fix the URLs here. The taxonomy list should be at /taxonomies/ and an individual taxonomy should be at e.g. taxonomy/4/, not taxonomy-list/4/. It doesn't make sense to have "list" in the URL for a single taxonomy.

Avoids clicking through to the card when using the menu button to hide a card's menu.

…l-page

* /taxonomy-list is now /taxonomies, and there's a temporary redirect * /taxonomy-list/:id: is now /taxonomy/:id:

pomegranited · 2023-11-20T03:36:38Z

@xitij2000 I've addressed your nits with 0e19e99.

@bradenmacdonald I fixed that menu bug with 3f0dbaa, and changed the URLs with a15e3a4.

Let me know if there's anything else we should fix before this can be merged?

arbrandes

Approved, but it seems a rebase is needed.

bradenmacdonald · 2023-11-20T19:58:17Z

I can rebase now.

openedx-webhooks · 2023-11-20T20:15:34Z

@rpenido 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

ChrisChV added 8 commits October 20, 2023 18:42

feat: System-defined tooltip added

4963124

feat: Taxonomy card menu added. Export menu item added

4077397

feat: Modal for export taxonomy

e87c545

feat: Connect with export API

4922420

test: Tests for API and selectors

5e12a8f

feat: Use windows.location.href to call the export endpoint

09aa8b6

test: ExportModal.test added

1925fae

style: Delete unnecesary code

0be804c

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 24, 2023

rpenido force-pushed the rpenido/fal-3529-bare-bones-taxonomy-detail-page branch 5 times, most recently from 965385f to 3d45969 Compare October 25, 2023 13:02

Merge branch 'master' into chris/FAL-3528-taxonomy-export-menu

da0156f

rpenido force-pushed the rpenido/fal-3529-bare-bones-taxonomy-detail-page branch 3 times, most recently from 5d927a9 to 87808bb Compare October 25, 2023 19:18

ChrisChV and others added 5 commits October 30, 2023 11:39

Merge branch 'master' into chris/FAL-3528-taxonomy-export-menu

0cc473c

docs: README updated with taxonomy feature

5af8d66

style: TaxonomyCard updated to a better code style

a21fac2

style: injectIntl replaced by useIntl on taxonomy pages and components

61864d3

refactor: Move and rename taxonomy UI components to match 0002 ADR

b09b86a

rpenido force-pushed the rpenido/fal-3529-bare-bones-taxonomy-detail-page branch 2 times, most recently from d408b52 to a12f262 Compare October 31, 2023 18:45

refactor: Move api to data to match with 0002 ADR

df8432d

bradenmacdonald mentioned this pull request Nov 3, 2023

[temp] feat: add detail taxonomy page open-craft/frontend-app-authoring#5

Closed

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Nov 6, 2023

mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Nov 9, 2023

rpenido force-pushed the rpenido/fal-3529-bare-bones-taxonomy-detail-page branch from 9ae8dc1 to c6b68bf Compare November 11, 2023 13:10

feat: add taxonomy detail page

56dbdd2

rpenido force-pushed the rpenido/fal-3529-bare-bones-taxonomy-detail-page branch from 568ee3e to 56dbdd2 Compare November 11, 2023 15:07

rpenido changed the title ~~feat: bare bones taxonomy detail page~~ feat: bare bones taxonomy detail page [FC-0036] Nov 15, 2023

pomegranited added 3 commits November 16, 2023 10:16

Merge branch 'master' into rpenido/fal-3529-bare-bones-taxonomy-detai…

9c582ed

…l-page

fix: address nits in PR review

9df1699

refactor: move data/selectors to data/apiHooks

0fdaa07

and fix tests to mock useQuery.

pomegranited approved these changes Nov 16, 2023

View reviewed changes

rpenido marked this pull request as ready for review November 16, 2023 01:18

xitij2000 reviewed Nov 17, 2023

View reviewed changes

bradenmacdonald mentioned this pull request Nov 17, 2023

feat: display all child tags in the "bare bones" taxonomy detail page [FAL-3529] [FC-0036] open-craft/frontend-app-authoring#10

Closed

pomegranited added 4 commits November 20, 2023 11:15

fix: address nits in PR review

0e19e99

fix: replace taxonomy menu ModalPopup with Dropdown menu

3f0dbaa

Avoids clicking through to the card when using the menu button to hide a card's menu.

Merge branch 'master' into rpenido/fal-3529-bare-bones-taxonomy-detai…

0872d5d

…l-page

fix: change taxonomy URLs

a15e3a4

* /taxonomy-list is now /taxonomies, and there's a temporary redirect * /taxonomy-list/:id: is now /taxonomy/:id:

arbrandes approved these changes Nov 20, 2023

View reviewed changes

chore: update this branch with master

924e28e

arbrandes merged commit 02cdccc into openedx:master Nov 20, 2023
5 checks passed

rpenido deleted the rpenido/fal-3529-bare-bones-taxonomy-detail-page branch November 21, 2023 12:59

kenclary mentioned this pull request Jan 9, 2024

fix: configure webpack to fix paid powerpaste plugin #783

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: bare bones taxonomy detail page [FC-0036] #655

feat: bare bones taxonomy detail page [FC-0036] #655

rpenido commented Oct 24, 2023 •

edited

Loading

openedx-webhooks commented Oct 24, 2023 •

edited

Loading

e0d commented Nov 6, 2023

bradenmacdonald commented Nov 15, 2023

bradenmacdonald commented Nov 15, 2023

pomegranited left a comment

pomegranited commented Nov 16, 2023

rpenido commented Nov 16, 2023

xitij2000 left a comment

xitij2000 Nov 17, 2023

pomegranited Nov 20, 2023 •

edited

Loading

xitij2000 Nov 17, 2023

pomegranited Nov 20, 2023

xitij2000 Nov 17, 2023

pomegranited Nov 20, 2023

xitij2000 Nov 17, 2023

pomegranited Nov 20, 2023

xitij2000 Nov 17, 2023

pomegranited Nov 20, 2023

bradenmacdonald commented Nov 17, 2023

bradenmacdonald commented Nov 17, 2023

pomegranited commented Nov 20, 2023

arbrandes left a comment

bradenmacdonald commented Nov 20, 2023

openedx-webhooks commented Nov 20, 2023

		@@ -0,0 +1 @@
		export { default as TagListTable } from './TagListTable'; // eslint-disable-line import/prefer-default-export

	export { default as TagListTable } from './TagListTable'; // eslint-disable-line import/prefer-default-export
	/* eslint-disable import/prefer-default-export */
	export { default as TagListTable } from './TagListTable';

feat: bare bones taxonomy detail page [FC-0036] #655

feat: bare bones taxonomy detail page [FC-0036] #655

Conversation

rpenido commented Oct 24, 2023 • edited Loading

Description

Supporting Information

Testing instructions

openedx-webhooks commented Oct 24, 2023 • edited Loading

e0d commented Nov 6, 2023

bradenmacdonald commented Nov 15, 2023

bradenmacdonald commented Nov 15, 2023

pomegranited left a comment

Choose a reason for hiding this comment

pomegranited commented Nov 16, 2023

rpenido commented Nov 16, 2023

xitij2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pomegranited Nov 20, 2023 • 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

bradenmacdonald commented Nov 17, 2023

bradenmacdonald commented Nov 17, 2023

pomegranited commented Nov 20, 2023

arbrandes left a comment

Choose a reason for hiding this comment

bradenmacdonald commented Nov 20, 2023

openedx-webhooks commented Nov 20, 2023

rpenido commented Oct 24, 2023 •

edited

Loading

openedx-webhooks commented Oct 24, 2023 •

edited

Loading

pomegranited Nov 20, 2023 •

edited

Loading