feature/deseng674: Implemented authoring tab to admin engagement view #2576

jareth-whitney · 2024-08-15T20:45:53Z

…. Fixed a couple of issues with Footer and Admin SideNav as well.

Issue #: https://citz-gdx.atlassian.net/browse/DESENG-674

Description of changes:

Implemented authoring tab in admin engagement view
Co-ordinated with Nat to build admin view index.tsx
- Used original data fetch config since I don't have Nat's loader
- Modified tab values to match hi-fi prototype
Fixed an invalid prop in the footer
Fixed a React key error in admin side nav

User Guide update ticket (if applicable):
N/A

…. Fixed a couple of issues with Footer and Admin SideNav as well.

codecov-commenter · 2024-08-15T20:57:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.04%. Comparing base (76fbfd4) to head (b9b2baa).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2576   +/-   ##
=======================================
  Coverage   76.04%   76.04%           
=======================================
  Files         609      609           
  Lines       22080    22080           
  Branches     1785     1785           
=======================================
  Hits        16790    16790           
  Misses       5026     5026           
  Partials      264      264

Flag	Coverage Δ
metweb	`65.02% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
met-web/src/components/layout/SideNav/SideNav.tsx	`94.33% <100.00%> (ø)`

…eys.

Baelx

Could we discuss the use of generateUniqueKey if you haven't found another solution yet? Also I commented in a few places where I was trying to understand the code. Since someone else may be updating the SystemMessage to work in the future, I just wanted to make sure I understood the intended use of it.

Baelx · 2024-08-15T21:24:41Z

met-web/src/components/engagement/admin/view/AuthoringTab.tsx

+    const defaultFeedbackMethods = getAuthoringValues(defaultAuthoringValue, feedbackTitles, true);
+
+    // Set useStates. When data is imported, it will be set with setSectionValues and setFeedbackMethods.
+    const [sectionValues, setSectionValues] = useState(defaultSectionValues);


Looks like you've set up some infrastructure here for the conditional display of the alert messages! I know this was a tricky ask since we don't actually have any engagement data to work with.

I'm trying to understand the code here:

It seems like the variables defined on lines 82-87 are just for showing the lists of sections to complete, correct?

Is sectionValues meant to contain actual engagement data? If so, why is it set to hold data that's meant for the rendering of the section lists? Was the idea to change it to real-live engagement data later?

Sorry, that's down to the way Prettier is autoformatting my code! Those are actually all arguments for the function, and then I'm returning the result of the map below that.

Baelx · 2024-08-15T21:26:10Z

met-web/src/components/engagement/admin/view/AuthoringTab.tsx

+
+    // Listen to the sectionValues useState and update the boolean value that controls the presence of the "sections" incomplete message.
+    useEffect(() => {
+        if (true !== requiredSectionsCompleted && allRequiredItemsComplete(sectionValues)) {


Just curious why we need both requiredSectionsCompleted and allRequiredItemsComplete here

allRequiredItemsComplete is a comparison function that returns a boolean value. It's the check that occurs before I assign the value to the requiredSectionsCompleted useState boolean.

Baelx · 2024-08-15T21:27:20Z

met-web/src/components/engagement/admin/view/AuthoringTab.tsx

+        paddingRight: '0.4rem',
+    };
+    return (
+        <Link style={{ textDecoration: 'none' }} href={props.item.link}>


A link is definitely the way to go for those section links! As oppose to a plain button

Yes definitely better for accessibility, as you taught me.

Baelx · 2024-08-15T21:27:55Z

met-web/src/components/engagement/admin/view/AuthoringTab.tsx

+    const [requiredSectionsCompleted, setRequiredSectionsCompleted] = useState(false);
+    const [feedbackCompleted, setFeedbackCompleted] = useState(false);
+
+    // Define styles


Thanks for grouping the styles together nicely!

Baelx · 2024-08-15T21:32:59Z

met-web/src/components/engagement/admin/view/AuthoringTab.tsx

+
+    // Listen to the feedbackMethods useState and update the boolean value that controls the presence of the "feedback" incomplete message.
+    useEffect(() => {
+        if (true !== feedbackCompleted && allRequiredItemsComplete(feedbackMethods)) {


Just curious about the need for both conditionals here too. Also, was there a reason you couldn't do:

Suggested change

if (true !== feedbackCompleted && allRequiredItemsComplete(feedbackMethods)) {

if (!feedbackCompleted && allRequiredItemsComplete(feedbackMethods)) {

To me the first way is a bit more readable and a bit more strict, but I could go either way.

Baelx · 2024-08-15T21:41:30Z

met-web/src/components/engagement/admin/view/AuthoringTab.tsx

+    // Check if all required items are completed.
+    const allRequiredItemsComplete = (values: AuthoringValue[]) => {
+        const itemChecklist = values.map((value) => {
+            if (undefined === value.required || undefined === value.completed) {


It looks like value.required and value.completed are explicitly set as falseto begin with. How would they get set toundefined`?

Right, you wouldn't have an undefined value from the default data. It's when you start introducing real data that this becomes important.

I'm okay with proceeding with this code but I thought I'd add a note that with Typescript, we often don't need to account for undefined popping up. We have a lot more type control which allows us to not have to worry about checking for undefined in case things break.

Baelx · 2024-08-15T22:00:53Z

met-web/src/components/engagement/admin/view/index.tsx

@@ -32,6 +42,52 @@ export const AdminEngagementView = () => {
                    )}
                </Await>
            </Suspense>
-        </div>
+            <TabContext value={currentTab}>
+                <TabList


Does this render as a nav? If not, we can specify the component element to use, or give it an aria-role of navigation

It renders as a div right now. I'll see if I can make it render as a nav instead.

Rendering as a nav element as of the latest commit

Baelx · 2024-08-15T22:02:37Z

met-web/src/utils/index.ts

@@ -81,3 +81,11 @@ export const isDarkColor = (color: string, sensitivity = 0.5) => {
    const luminance = (0.299 * r + 0.587 * g + 0.114 * b) / 255;
    return luminance < sensitivity;
 };
+
+export const generateUniqueKey = () => {


As discussed a bit, let's review the use of this function as a "key" generator. There are other recommended methods for this

Baelx · 2024-08-15T22:06:54Z

met-web/src/components/engagement/admin/view/AuthoringTab.tsx

        title: '',
        link: '#',
        required: false,
        completed: false,
    };
-    const getAuthoringValues = (defaultValues: AuthoringValue, titles: string[], required: boolean): AuthoringValue[] =>
-        titles.map((title) => ({ ...defaultValues, title: title, required: required }));
+    const getAuthoringValues = (


Thanks for changing the formatting! It's easier on the eyes

…Tab component.

sonarcloud · 2024-08-15T23:08:05Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

feature/deseng674: Implemented authoring tab to admin engagement view…

5b2d98c

…. Fixed a couple of issues with Footer and Admin SideNav as well.

jareth-whitney requested review from Baelx and NatSquared August 15, 2024 20:45

feature/deseng674: Removed random number generator for unique React k…

a363bf6

…eys.

Baelx requested changes Aug 15, 2024

View reviewed changes

Baelx reviewed Aug 15, 2024

View reviewed changes

jareth-whitney added 3 commits August 15, 2024 15:09

feature/deseng674: Removed unused useState set functions in Authoring…

c59e3f4

…Tab component.

feature/deseng674: Changed component type of tabs to a nav element.

0b33749

feature/deseng674: Updated changelog.

b9b2baa

Baelx approved these changes Aug 15, 2024

View reviewed changes

jareth-whitney merged commit ee6caab into main Aug 15, 2024
8 checks passed

jareth-whitney deleted the feature/deseng674 branch August 15, 2024 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/deseng674: Implemented authoring tab to admin engagement view #2576

feature/deseng674: Implemented authoring tab to admin engagement view #2576

jareth-whitney commented Aug 15, 2024

codecov-commenter commented Aug 15, 2024 •

edited

Loading

Baelx left a comment

Baelx Aug 15, 2024

jareth-whitney Aug 15, 2024

Baelx Aug 15, 2024

jareth-whitney Aug 15, 2024 •

edited

Loading

Baelx Aug 15, 2024

jareth-whitney Aug 15, 2024

Baelx Aug 15, 2024

Baelx Aug 15, 2024

jareth-whitney Aug 15, 2024

Baelx Aug 15, 2024

jareth-whitney Aug 15, 2024

Baelx Aug 16, 2024

Baelx Aug 15, 2024

jareth-whitney Aug 15, 2024

Baelx Aug 15, 2024

jareth-whitney Aug 15, 2024

Baelx Aug 15, 2024

Baelx Aug 15, 2024 •

edited

Loading

sonarcloud bot commented Aug 15, 2024

	if (true !== feedbackCompleted && allRequiredItemsComplete(feedbackMethods)) {
	if (!feedbackCompleted && allRequiredItemsComplete(feedbackMethods)) {

feature/deseng674: Implemented authoring tab to admin engagement view #2576

feature/deseng674: Implemented authoring tab to admin engagement view #2576

Conversation

jareth-whitney commented Aug 15, 2024

codecov-commenter commented Aug 15, 2024 • edited Loading

Codecov Report

Baelx 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

jareth-whitney Aug 15, 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

Baelx Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Aug 15, 2024

Quality Gate passed

codecov-commenter commented Aug 15, 2024 •

edited

Loading

jareth-whitney Aug 15, 2024 •

edited

Loading

Baelx Aug 15, 2024 •

edited

Loading