Conversion of Service Dialogs Form from Angular to React

[elsamaryv]

• Revamp of service dialogs (Also includes its conversion from Angular to React)

@miq-bot add-label enhancement
@miq-bot add-label wip

[Fryguy]

Are these for "real" notifications or for debug-type notifications? If the latter, then this shouldn't be needed because if you run the webpack-dev-server and the rails server with CYPRESS=true then those debug-type notifications won't be there.

[Fryguy]

Because there are commands being defined globally, but are specific to service dialogs, I think the names should be prefixed with something like serviceDialog to avoid any name collisions and to clarify their purpose.

[Fryguy]

well, maybe not this one because it also includes login and navigation

[Fryguy]

I think we generally sort this list alphabetically (unless there's some internal dependency reason to not do that)

[Fryguy]

Same on sorting.

[Fryguy]

I know this is still WIP, but can @GilbertCherrie , @asirvadAbrahamVarghese please review to get a first pass on it?

[asirvadAbrahamVarghese]

I think in React-17+, JSX works without importing React, so we should be able to drop it post our upgrade 👍

[asirvadAbrahamVarghese]

• if it’s just to pass through any actions required for the close button click maybe we could rename the prop to something like onCloseClick (I’m clueless when it comes to naming, so it’s your call)
• Should we consider changing message to messageInfo, given that its an object? - again thats your call

[elsamaryv]

if it’s just to pass through any actions required for the close button click maybe we could rename the prop to something like onCloseClick

This component pulls InlineNotification component from carbon-components-react and is used to display contextual messages inline with other contents in the UI. onCloseButtonClick is actually a property of the InlineNotification component - It is an event handler that is only called when the user clicks the close button(if it is visible).

[asirvadAbrahamVarghese]

I meant the setMessage prop from our component that we’re using as the handler for onCloseButtonClick in InlineNotification, I feel like onCloseButtonClick ={onCloseClick} (or anything similar) seems better in this context

[asirvadAbrahamVarghese]

It might be better to control hideCloseButton through a separate boolean prop and set a default for it 🤔

[asirvadAbrahamVarghese]

I was thinking if we should directly do:
onCloseButtonClick={setMessage}
So that react will pass down the event automatically when setMessage handler is called similar to how we’d pass the event manually like:
onCloseButtonClick={(event)=>setMessage(event)}

So if in case we want to access any event methods (preventDefault, stopPropagation) from this component sometime later we can do:
<InlineFlashMessage setMessage={(event)=>{ event.preventDefault(); }} .... />

[asirvadAbrahamVarghese]

since we are doing if (!message) return null; at the top of the component, we can safely skip setting message as null from default props unless its marked required by PropTypes.shape({}).isRequired - the value received will be undefined and would still match our if(!message) condition

[asirvadAbrahamVarghese]

We can set a no-op function as default instead of undefined:
setMessage: () => {},
If thats throwing any eslint error(empty blocks), we can do:

  setMessage: () => {
    // default callback
  },

so that it prevents errors like “Cannot call setMessage of undefined”.
this also means we don’t have to explicitly verify typeof setMessage === 'function' before passing existing function reference directly: onCloseButtonClick={setMessage}
or even if we are creating a new arrow function on every render that calls onCloseButtonClick:

      onCloseButtonClick={() => {
        // any other action
        setMessage();
      }}

[asirvadAbrahamVarghese]

Correct me here if I am wrong
We will be using the component like:

        <CustomDateTimePicker
          label="Custom Date Time Picker"
          initialData={{ date: '11/06/2025', time: '23:11', period: 'PM' }}
          onChange={(data) => {
            // any actions with on change data
          }}
        />

So I was thinking - just like we did with InlineFlashMessage, inline destructuring of component props is generally cleaner instead of accessing them via field object:
const CustomDateTimePicker = ({ label, onChange, initialData }) => {...

and then add props validations and default props for them:

CustomDateTimePicker.propTypes = {
  initialData: PropTypes.shape({
    date: PropTypes.string,
    time: PropTypes.string,
    period: PropTypes.string,
  }),
  onChange: PropTypes.func,
  label: PropTypes.string,
};

CustomDateTimePicker.defaultProps = {
  initialData: { date: '', time: '', period: '' },
  onChange: () => {},
  label: '',
};

Note: date, time and period having an empty string as default value ({ date: '', time: '', period: '' }) shouldn't be a problem as they are falsy in JS, fallback methods like getCurrentDate and getCurrentTimeAndPeriod will still be triggered to set states initial values

[asirvadAbrahamVarghese]

I guess it would be better to call the function immediately (getCurrentDate) instead of passing the reference for React to call it automatically to get the initial state value when initialData.date is not available:
const [date, setDate] = useState(() => initialData.date || getCurrentDate());

also include the lazy initialization () =>🔝, like we did with time and period states initializations, so that the arrow function is only executed during the initial render, on subsequent re-renders, react knows it already has a state value and doesn't need to call the function again. The performance difference would be negligible here I believe, but it's better to use the arrow function for any initialization that creates objects, makes calculations or anything that could potentially be expensive.

[asirvadAbrahamVarghese]

On initial render, time would look like this which is not in our expected "hh:mm" format, it should have been "09:54", But the invalid time error: "Enter a valid 12-hour time" is not displayed, probably thats because isValid state is initially true

Right after time field is edited the error is displayed since our state isValid state is now updated:

No issues with this implementation, this should work well once we apply zero-padding to the hour value in getCurrentTimeAndPeriod(similar to what we did with minutes).
It should be padded after converting to 12-hour format, otherwise, the modulo will reduce it to a single digit again.
hours = ${hours % 12 || 12}.padStart(2, '0');
Which should work well:

[asirvadAbrahamVarghese]

if (newDate.length) should be fine here since 0 is also falsy in JS and any number greater than 0 will be truthy

[asirvadAbrahamVarghese]

Tried "11:5" in the time field, which doesn't match our pattern, so it's treated as an invalid time, the regex correctly returns false, which we also assign to the isValid state:

Given the condition(if (isValid) onChange({ ... });), we don't expect the onChange prop to be triggered in this case but it still gets called since isValid is still true(initial state value is true):

This is because state updates are asynchronous & batched and won't be reflecting immediately, when setIsValid is called react schedules the state update and updated value will be available only in the next render(only after the current event scope) not immediately after the call. So within the same event, we will still be accessing the previous state(which is true).

Returning the regex result from validateTime and using it for both setting state and controlling onChange should work fine:

  const validateTime = (value) => {
    const timeRegex = /^(0[1-9]|1[0-2]):[0-5][0-9]$/; // Matches 12-hour format hh:mm
    return timeRegex.test(value);
  };
  const handleTimeChange = (event) => {
    const newTime = event.target.value;
    setTime(newTime);
    const isValidTime = validateTime(newTime);
    setIsValid(isValidTime);
    if (isValidTime) onChange({ value: combinedDateTime(), initialData });
  };

[asirvadAbrahamVarghese]

The combinedDateTime function is also impacted the same way, we are updating the time state from here & then trying to access the state value from the same event scope, we will be having the previous time value due to react's batched updates.
Time field value is updated to "12:53" but time state will still provide its previous value "12:5" inside the same method scope:

Instead of relying on stale state we could update combinedDateTime to use the same input we're using for the state (in a way that it uses parameter value if available or the state value, thus other state values(date & period) are set):

  const combinedDateTime = ({ dateValue, timeValue, periodValue } = {}) =>
    `${dateValue || date} ${timeValue || time} ${periodValue || period}`;

🔝 we can also avoid the intermediate variable(const dateTime) by returning the string immediately

Now from handleTimeChange we can pass the value for time like:

    if (isValidTime) {
      onChange({
        value: combinedDateTime({ timeValue: newTime }),
        initialData,
      });
    }

So that correct updated time is returned from combinedDateTime

[asirvadAbrahamVarghese]

Like discussed below, due to batched state updates we might want to update combinedDateTime to receive the updated date string:

  const combinedDateTime = ({ dateValue, timeValue, periodValue } = {}) =>
    `${dateValue || date} ${timeValue || time} ${periodValue || period}`;

and then from handleDateChange pass the date string value:

  const handleDateChange = (newDate) => {
    if (newDate.length) {
      const formattedDate = new Intl.DateTimeFormat('en-US', {
      .....
      onChange({
        value: combinedDateTime({ dateValue: formattedDate }),
        initialData,
      });
    }
  };

[asirvadAbrahamVarghese]

Same as above, due to batched state updates we might want to update combinedDateTime to receive the updated period string:

  const combinedDateTime = ({ dateValue, timeValue, periodValue } = {}) =>
    `${dateValue || date} ${timeValue || time} ${periodValue || period}`;

and then from handlePeriodChange pass the updated period value:

  const handlePeriodChange = (event) => {
    const newPeriod = event.target.value;
    setPeriod(newPeriod);
    onChange({
      value: combinedDateTime({ periodValue: newPeriod }),
      initialData,
    });
  };

[asirvadAbrahamVarghese]

Is the label prop expected to always be provided? The label element still gets rendered in the DOM even when no label is provided:

conditionally rendering it can help prevent unused elements in the DOM:
{label && <FormLabel>{label}</FormLabel>}
So that Label won't be rendered:

[asirvadAbrahamVarghese]

Dealing with dates can be bug-prone. we should be fine for now with simple date logics but as logic grows, pulling in a utility like date-fns would help.