Adding end date to event card #10

jenbreese · 2024-03-28T22:47:37Z

READY FOR REVIEW

Feedback needed.
I want the end date to appear when it is appropriate. I took some of the code from the card and tried to change it to a ternary. It isn't right. Can you give me feedback?
I would like it to look like this:

Summary

TL;DR - what's this PR for?

Review By (Date)

When does this need to be reviewed by?

Criticality

How critical is this PR on a 1-10 scale? Also see Severity Assessment.
E.g., it affects one site, or every site and product?

Urgency

How urgent is this? (Normal, High)

Review Tasks

Setup tasks and/or behavior to test

Check out this branch
Rebuild Cache and import config drush cr ; drush ci
Navigate to...
Verify...

Site Configuration Sync

Is there a config:export in this PR that changes the config sync directory?

Front End Validation

Design is approved by @ user?
HTML validation: Is the markup using the appropriate semantic tags and passes validation? Or, QA request ticket created?
Cross-browser testing: Has been performed? Or, QA request ticket created?
Automated accessibility: Scans performed? Or, QA request ticket created?
Manual accessibility: Manually tested? Or, QA request ticket created?

Backend / Functional Validation

Code

Are the naming conventions following our standards?
Does the code have sufficient inline comments?
Is there anything in this code that would be hidden or hard to discover through the UI?
Are there any code smells?
Are tests provided? eg (unit, behat, or codeception)

Code security

Are all forms properly sanitized?
Any obvious security flaws or new areas for attack?

General

Is there anything included in this PR that is not related to the problem it is trying to solve?
Is the approach to the problem appropriate?

Affected Projects or Products

Does this PR impact any particular projects, products, or modules?

Associated Issues and/or People

JIRA ticket(s)
Other PRs
Any other contextual information that might be helpful (e.g., description of a bug that this PR fixes, new functionality that it adds, etc.)
Anyone who should be notified? (@mention them here)

Resources

AMP Tool
Accessibility Manual Test Script
HTML Validator
Browserstack and link to Browserstack Credentials

pookmish · 2024-03-28T22:52:41Z

src/components/nodes/cards/stanford-event/stanford-event-card.tsx

  // Fix difference between server side render and client side render. Replace any strange characters.
  const dateTimeString = getEventTimeString(start, end, timeZone).replace(/[^a-zA-Z0-9 ,:\-|]/, ' ');
  const Heading = headingLevel === 'h3' ? H3 : H2;
+
+  const endDate = "<span className='relative font-normal leading-trim top-7 text-m0 px-03em' aria-hidden='true'>– to –</span><span className='sr-only'>to</span><time dateTime='2023-07-03 00:00Z' className='flex flex-col'><span className='text-m0 font-semibold w-full text-center'>{endMonth.toUpperCase()}</span><span className='text-m4 font-bold w-full text-center'>{endDay}</span></time>"


We shouldn't put markup into variables like this. You should always put the markup directly into the component and use conditional rendering:

{someField && <div>{someField}</div> }

Optionally, you could create a separate component for just the date display area.

pookmish · 2024-04-05T18:13:08Z

src/components/nodes/cards/stanford-event/stanford-event-card.tsx

-          {startDay}
-        </div>
+      <div aria-hidden className="flex w-fit justify-start  flex-row items-center min-w-[9rem] h-90">
+        <time dateTime="2023-06-24 00:00Z" className="flex flex-col">


This time should be dynamic based on the start date and it should be lowercase datetime. See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/time#usage_notes for proper data formatting

pookmish · 2024-04-05T18:13:23Z

src/components/nodes/cards/stanford-event/stanford-event-card.tsx

-        </div>
+      <div aria-hidden className="flex w-fit justify-start  flex-row items-center min-w-[9rem] h-90">
+        <time dateTime="2023-06-24 00:00Z" className="flex flex-col">
+          <span className="text-m0 font-semibold w-full text-center"> {startMonth.toUpperCase()}</span>


extra space

Suggested change

<span className="text-m0 font-semibold w-full text-center"> {startMonth.toUpperCase()}</span>

<span className="text-m0 font-semibold w-full text-center">{startMonth.toUpperCase()}</span>

pookmish · 2024-04-05T18:19:25Z

src/components/nodes/cards/stanford-event/stanford-event-card.tsx

+        {(startDay != endDay) && (startMonth != endMonth) ? 
+        <><span className='relative font-normal leading-trim top-7 text-m0 px-03em' aria-hidden='true'>– to –</span><span className='sr-only'>to</span><time dateTime='2023-07-03 00:00Z' className='flex flex-col'>
+          <span className='text-m0 font-semibold w-full text-center'>{endMonth.toUpperCase()}</span>
+          <span className='text-m4 font-bold w-full text-center'>{endDay}</span>
+         </time>
+        </>
+        : null
+        }


This logic is hard to read. you should be able to simplify it and fix indentations & lines. And we don't need the sr-only text since this whole region is aria-hidden.
There's no need for the time element since we already included that for the start time.

Suggested change

{(startDay != endDay) && (startMonth != endMonth) ?

<><span className='relative font-normal leading-trim top-7 text-m0 px-03em' aria-hidden='true'>– to –</span><span className='sr-only'>to</span><time dateTime='2023-07-03 00:00Z' className='flex flex-col'>

<span className='text-m0 font-semibold w-full text-center'>{endMonth.toUpperCase()}</span>

<span className='text-m4 font-bold w-full text-center'>{endDay}</span>

</time>

</>

: null

}

{(startDay != endDay && startMonth != endMonth) &&

<>

<span className="relative font-normal leading-trim top-7 text-m0 px-03em">– to –</span>

<div className="flex flex-col">

<span className="text-m0 font-semibold w-full text-center">{endMonth.toUpperCase()}</span>

<span className="text-m4 font-bold w-full text-center">{endDay}</span>

</div>

</>

}

…o story

pookmish · 2024-04-09T14:19:43Z

src/components/nodes/cards/stanford-event/stanford-event-card.tsx

+        {
+        (startDay != endDay)
+        ||
+        (startMonth != endMonth)
+        ? 
+        <>


You should try to avoid {isTrue ? <div>something</div> : <div>something else</div>}. it becomes difficult to read. Especially if the "something else" is null, there's no reason for such structure. This can be simplified to

{(startDay != endDay || startMonth != endMonth) && <div>some result</div> }

pookmish · 2024-04-09T14:20:46Z

src/components/nodes/cards/stanford-event/stanford-event-card.tsx

+        (startMonth != endMonth)
+        ? 
+        <>
+          <span className='relative font-normal leading-trim top-7 text-m0 px-03em' aria-hidden='true'>– to –</span><span className='sr-only'>to</span>


Please replace all single quotes with double quotes. If you merge in the latest 1.x branch, you should get IDE notifications. Also yarn lint is always a good tool to check.

pookmish · 2024-04-09T14:22:46Z

src/components/nodes/cards/stanford-event/stanford-event-card.tsx

+        (startMonth != endMonth)
+        ? 
+        <>
+          <span className='relative font-normal leading-trim top-7 text-m0 px-03em' aria-hidden='true'>– to –</span><span className='sr-only'>to</span>


Don't need aria-hidden here since the parent container already has aria-hidden

pookmish · 2024-04-09T14:23:33Z

src/components/nodes/list-item/stanford-event/stanford-event-list-item.tsx

        <div className="text-m0 font-semibold mb-4 w-full text-center">
          {startMonth.toUpperCase()}
        </div>
        <div className="text-m4 font-bold w-full text-center">
          {startDay}
        </div>
+        {


All the same feed back as the card.

pookmish · 2024-09-09T17:06:32Z

I've made a separate commit to get this over the line. Closing this since it's completed elsewhere

adding end date to event card

2db8404

jenbreese requested a review from pookmish March 28, 2024 22:47

jenbreese assigned pookmish Mar 28, 2024

pookmish requested changes Mar 28, 2024

View reviewed changes

jenbreese added 4 commits March 28, 2024 23:28

in progress changes

a930bd5

removing the seperation

f869dd0

removing extra unused variable added

fae6a52

removed include and added parans

1fbd2c1

pookmish requested changes Apr 5, 2024

View reviewed changes

jenbreese added 7 commits April 8, 2024 14:53

merge

a62860a

fixup

5618ee2

fix to lint error

bcfd3f1

another string with double quotes

b196088

fix to add month back

c332b2f

fixed the list of events with multiple dates and added date control t…

50309db

…o story

fix to dateTime

ccfc4bb

pookmish requested changes Apr 9, 2024

View reviewed changes

pookmish force-pushed the 1.x branch 2 times, most recently from 6524635 to a9f4c29 Compare July 10, 2024 19:32

pookmish closed this Sep 9, 2024

pookmish deleted the event-card-end-date branch September 9, 2024 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding end date to event card #10

Adding end date to event card #10

jenbreese commented Mar 28, 2024

pookmish Mar 28, 2024

pookmish Apr 5, 2024

pookmish Apr 5, 2024

pookmish Apr 5, 2024

pookmish Apr 9, 2024

pookmish Apr 9, 2024

pookmish Apr 9, 2024

pookmish Apr 9, 2024

pookmish commented Sep 9, 2024

	<span className="text-m0 font-semibold w-full text-center"> {startMonth.toUpperCase()}</span>
	<span className="text-m0 font-semibold w-full text-center">{startMonth.toUpperCase()}</span>

-        {(startDay != endDay) && (startMonth != endMonth) ?
-        <><span className='relative font-normal leading-trim top-7 text-m0 px-03em' aria-hidden='true'>– to –</span><span className='sr-only'>to</span><time dateTime='2023-07-03 00:00Z' className='flex flex-col'>
-          <span className='text-m0 font-semibold w-full text-center'>{endMonth.toUpperCase()}</span>
-          <span className='text-m4 font-bold w-full text-center'>{endDay}</span>
-         </time>
-        </>
-        : null
-        }
+        {(startDay != endDay && startMonth != endMonth) &&
+        <>
+          <span className="relative font-normal leading-trim top-7 text-m0 px-03em">– to –</span>
+          <div className="flex flex-col">
+            <span className="text-m0 font-semibold w-full text-center">{endMonth.toUpperCase()}</span>
+            <span className="text-m4 font-bold w-full text-center">{endDay}</span>
+          </div>
+        </>
+        }

Adding end date to event card #10

Adding end date to event card #10

Conversation

jenbreese commented Mar 28, 2024

READY FOR REVIEW

Summary

Review By (Date)

Criticality

Urgency

Review Tasks

Setup tasks and/or behavior to test

Site Configuration Sync

Front End Validation

Backend / Functional Validation

Code

Code security

General

Affected Projects or Products

Associated Issues and/or People

Resources

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

pookmish commented Sep 9, 2024