Chameleon test

menu.jsx

/*
Thanks for your interest in a Frontend engineering role at Chameleon. We're a solid and collaborative team that
  will always lend a helping hand. We prioritize asynchronousity in nearly all of our ways of working.
This exercise is meant to be a focused, high signal ~hour. Assume that if we've asked about it it's important to us. We will
  evaluate your React skills, your general understanding of JavaScript, your creativity, organization, etc.
Prompt:

  0. How are you today? 😊
  R: doing good 😄
  1. Below are the parts of a basic selection menu, but there are issues and it's too specific for the single
     current use case. We all more or less agree it's not that great and needs some attention. It should be
     componentized/generalized so that other engineers on the team can use selection menus. We will evaluate your React
     skills, your general understanding of JavaScript, your creativity, organization and clarity on communication
  2. The following items (3,4,5) combined are intentionally sightly vague in the sense there’s often more than a single
     possible/correct solution — a natural aspect of software engineering. Please consider possible implementation paths
     you'd consider valid and pick the best one.
  3. Now, make improvements to the menu; in the end it should meet the prompt and be able to be used in at least a
     couple different use cases such as an Account picker, a searchable list of items, one step of a flow
     to configure Alerting, etc...
  4. For certain use cases we would want to sync the menu selection to the server. We have an abstraction for syncing called
     httpPatch. It can be used like this => httpPatch('user', { [`menu-state-${key}`]: {true,false} }). Where would this
     be included and how/where best to handle this (properties needing to be synced/loaded to the backend)?

    R: on both cases (getDropDownState and toogleSelection on dropdown item) for the asyncronous call it would be advisable to not include them on the component code for the sake of scallability, since the responsibility
       to fetch data should not be included on the component code , but rather on the page/container, better yet using a global state management lib like redux, or simply use react context.
       for the case of simplicity I added a onItemToggle function that can call the external function (or other function) if the developer needs (as explained on the code)



  5. Assuming others will use/add to this component in the future, write a 2-3 sentence "release note"
     to post into the #engineering channel in Slack (or add to the Components wiki page).

      R: # DropDownButton
         * usign the dropdown requires the use of `userId`, `items` properties (more information on the component code)
         * dropdown can search for items by it's `label` property using `searchEnabled` property or create a fine tunning search with `onSearch` property
         * dropdown can be synced with the server whenever something is selected if `itemKey` property is given to the `item` inside `items` (more information on the component code)

PS: No need to worry about CSS, actually making it "run" etc...
Now put youself in PR review mode...
  1. Please provide a couple Pull Request comments/suggestions to the engineer who wrote the code
     in pr-1.js; assume you're codeowner for the project on GitHub.
  2. Based on what you know from 1., how would you do this differently if you were
     to make this component from scratch?

  R: I would have added the current user information as property on the component, would have used a more readable, less imperative
  solution to the problem (used a filter instead of for loops), I would have also included unity tests to guarantee the component behavior,
  besides that I would have included ui information for when there is no data to be displayed

 */

import React, {useEffect, useState, useCallback} from 'react';
import {httpGet} from 'lib/http';

/**
 *
 * @typedef Item
 * @prop {string} label
 * @prop {string} id
 * @prop {string} itemKey
 * @prop {string} url
 */

/**
 *
 * @typedef Props
 * @prop {string} label
 * @prop {string} userId
 * @prop {Item[]} items
 * @prop {boolean} [searchEnabled]
 * @prop {()=>void} [onSearch]
 * @prop {()=>void} [onItemToggle]
 */

/**
   * Component To show a list of items to be selected after clicking on a button
   * @example
   * const items = [
      {label: 'page1', id: '1', itemKey: 'page1', url: '/page1'},
      {label: 'page2', id: '2', itemKey: 'page2', url: '/page2'},
      {label: 'page3', id: '3', itemKey: 'page3', url: '/page3'},
      {label: 'page4', id: '4', itemKey: 'page4', url: '/page4'},
      {label: 'page5', id: '5', itemKey: 'page5', url: '/page5'},
      {label: 'page6', id: '6', itemKey: 'page6', url: '/page6'},
    ];
   * const syncSelection = (itemKey, isSelected) => httpPatch('user', {[`menu-state-${itemKey}`]: isSelected});
   *
   * <Dropdown label="Pages", userId={456} items={items} onItemToggle={onItemToggle}/>
   *
   * @param {Props} props
   */
export const Dropdown = ({
  label,
  userId,
  items,
  searchEnabled = false,
  onItemToggle,
  onSearch,
}) => {
  const [isOpen, setIsOpen] = useState(false);
  const [filteredItems, setFilteredItems] = useState();
  const currentItems = filteredItems || items;

  /*
    a useEffect call was added so the request to sync the dropdown information is called only once when the component renders, also in conjunction with the useCallback it's guaranteed
    to only make a request again (while the component is rendered) if the userId is changed
    */
  useEffect(() => {
    getDropDownState();
  }, [getDropDownState]);

  const getDropDownState = useCallback(() => {
    httpGet(`users/${userId}`).then(d => {
      const {user} = d;
      setIsOpen(user[`dropdown_${user.name}`]);
    });
  }, [userId]);

  // this is just a simple implementation of a search for items based on the label property, if a more fine tuned search is needed the developer pass the onSearch function and implement
  // it based on the developer needs (for example for external asyncronous search or a search based on more properties than label)
  const search = text => {
    if (onSearch) {
      onSearch(text);
      return;
    }
    setFilteredItems(items.filter(item => item.label === text));
  };

  const onToggle = e => {
    setIsOpen(isOpen);
  };

  return (
    <>
      <div className="dropdown">
        <button
          type="button"
          className="dropdown-button"
          id="dropdownButton"
          aria-haspopup="true"
          aria-expended={isOpen}
          onClick={onToggle}>
          {label}
        </button>
        <div>
          {searchEnabled && (
            <input
              placeholder="Search Items"
              onChange={e => search(e.target.value)}
            />
          )}
          <ul
            className={`${
              isOpen ? 'dropdown-open' : ''
            } dropdown-menu dropdown-section`}
            aria-labelledby="dropdownButton"
            role="menu">
            <div>Items</div>
            {currentItems.slice(0, 4).map(item => (
              <DropdownItem
                key={item.id}
                href={item.url}
                itemKey={item.key}
                onItemToggle={onItemToggle}>
                {item.label}
              </DropdownItem>
            ))}
          </ul>
          {!!currentItems?.slice(4).length && (
            <ul
              className={`${
                isOpen ? 'dropdown-open' : ''
              } dropdown-menu dropdown-section`}>
              <div>More items</div>
              {currentItems?.slice(4).map(item => (
                <DropdownItem
                  key={item.id}
                  href={item.url}
                  itemKey={item.key}
                  onItemToggle={onItemToggle}>
                  {item.label}
                </DropdownItem>
              ))}
            </ul>
          )}
        </div>
      </div>
    </>
  );
};

const DropdownItem = ({children, href, itemKey, onItemToggle}) => {
  const [isSelected, setIsSelected] = useState(false);
  const toogleSelection = () => {
    setIsSelected(!isSelected);
    onItemToggle(itemKey, isSelected);
    /* the developer can then implement the patch function like this
       (itemKey, isSelected) => httpPatch('user', {[`menu-state-${itemKey}`]: isSelected});
      */
  };
  return (
    <li>
      <a href={href} onClick={toogleSelection}>
        {children}
      </a>
    </li>
  );
};

pr-1.js

// definetly not advisable to add data on window object, mainly because you lose control of state of application, better get this from a global state library/context and send
// this information as props for the components

window.currentUser = { id: '19', name: 'Jane', email: 'jane@chameleon.io' };

// users are like authors and profiles like commentors
// open a modal with commentor info when clicked

...

export const  ActiveProfiles({ profiles, onLaunchProfile }) => {
  var active = [];


/* 
it's dificult to read what the for loop does its better to separate in pieces of code, also the timeframe/islast24 function/constant better be added on a utils file/folder to be reusable
One way to improve the for loop initial implementation is to create a filter loop, that way we can even remove the if statement that comes after the for loop and doing all the checks/formating
of data at once 

```
const timeframe = new Date(new Date().getTime()-(24*60*1000)).toISOString()
const isLast24 = (profile) =>  profiles['last_seen_time'] > timeframe


const active = !!profiles.length && profiles.filter(profile => !profile.disable && isLast24(profile) && profile.email !== window.currentUser.email)
```
*/

  for(i=0; i < profiles.length; i++) {
    if(!profiles[i].disabled && profiles[i]['last_seen_time'] > new Date(new Date().getTime()-(24*60*1000)).toISOString()) { // within the last 24 hours
      active.push(profiles[i]);
    }
  }
  
  if(active.length == 1 && active[0].email  === window.currentUser.email) {
    active.length = 0;
  }


/* 
  here if there is not data on the active array it will show nothing to the user, better have text for when there is no data like so:
```
  !!active.length ?
  active.map(function(a) { return <div onClick={() => onLaunchProfile(a.name, a.email)}>{a.name} - {a.email}</div> })
  : <h3> No active profiles </h3>
```

*/

  return (
    <div>
       {active.map(function(a) { return <div onClick={() => onLaunchProfile(a.name, a.email)}>{a.name} - {a.email}</div> })}
    </div>
  )
}