added search and filter components by DeRaowl · Pull Request #52 · RealDevSquad/feature-flag-service

DeRaowl · 2022-07-16T15:40:12Z

Description

Fixes #32

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Manual testing
Test B // TODO

Test Configuration:

Firmware version:
Hardware:
Toolchain:
SDK:

Checklist:

My code follows the style guidelines of this project
[X ] I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

Neha

WIP review...

Neha · 2022-07-17T15:43:15Z

ui/src/App.js

+import Dashboard from './Components/Dashboard/Dashboard';
+import Filters from './Components/Filters/Filters';
+import Search from './Components/Search/Search';
+import { useFilterContext } from './Context/filter_context';


The naming convention of the filter_context is not as per the pattern of the project. Please use the Pascal case

Renamed filter_context to FilterContext

Neha · 2022-07-17T15:43:43Z

ui/src/Context/filter_context.js

@@ -0,0 +1,60 @@
+import { createContext, useContext, useEffect, useReducer } from 'react';
+import { FEATURE_FLAGS, FEATURE_FLAGS_HEADERS } from '../Mock/featureFlags';
+import { filterReducer } from '../reducers/filter_reducer';


The naming convention of the filter_reducers and reducers is not as per the pattern of the project. Please use the Pascal case

Converted file name to Pascal case

Neha · 2022-07-17T15:44:17Z

ui/src/reducers/filter_reducer.js

+  CLEAR_FILTER,
+  LOADING,
+  FILTER_LIST,
+} from '../actions/actions';


The naming convention of the action folder is not as per the pattern of the project. Please use the Pascal case

Converted filename to pascal case

Neha · 2022-07-17T15:49:09Z

ui/src/Components/Search/Search.js

+        name='userInput'
+        value={userInput}
+        onChange={handleInputChange}
+        placeholder='Search issues by Name, Createdby, Repository...'


This should come from the constant file

Added CONST file for placeholder

Neha · 2022-07-17T15:53:38Z

ui/src/Components/Search/Search.js

@@ -0,0 +1,47 @@
+import { useState, useCallback } from 'react';


We are not using useState, and useCallback in the component. Please remove it

Removed unused imports

Neha · 2022-07-17T15:59:56Z

ui/src/Components/Filters/Filters.js

+
+export default function Filters() {
+  const {
+    filterByEnabled,


filterByEnabled is not getting used in this component please remove it.

Removed unused code

Neha · 2022-07-17T16:00:51Z

ui/src/Components/Filters/Filters.js

+        name='status'
+        onChange={handleValueChange}
+      >
+        <option value='all'>Both</option>


Should move to the constant

Any reason it is not looking like dropdown

Added dropdown

@DeRaowl Ik the search feature is already implemented, What do you think about having these filters right below the headers? Like the one react-table offers. Just a thought, Let me know what you think?

CC: @Neha

ui/src/App.js

Neha · 2022-07-17T16:51:13Z

ui/src/App.js

+import Filters from './Components/Filters/Filters';
+import Search from './Components/Search/Search';
+import { useFilterContext } from './Context/filter_context';
+import { FEATURE_FLAGS, FEATURE_FLAGS_HEADERS } from './Mock/featureFlags';


Not getting used in the component. Please remove

Neha · 2022-07-17T16:58:20Z

ui/src/App.js

+
+  if (isLoading) {
+    // We can add spinner here - TODO
+    return <div>Loading...</div>;


This is not accessible content. We should :

Use text tag eg: heading, P, span

We might need to put the focus of the tab on loading text later (it will be decided how our screen is coming). For now, just do point 1

Used span tag instead of div

Neha · 2022-07-17T20:05:42Z

ui/src/actions/actions.js

@@ -0,0 +1,5 @@
+export const UPDATE_FILTERS = 'UPDATE_FILTERS';


The naming convention of this file should be filters.action.js

renamed file as per suggestion

Neha · 2022-07-17T20:09:25Z

ui/src/Components/Filters/Filters.js

+        name='status'
+        onChange={handleValueChange}
+      >
+        <option value='all'>Both</option>


Any reason it is not looking like dropdown

Neha · 2022-07-17T20:11:07Z

ui/src/App.js

-      />
+    <main className='flex flex-col items-center'>
+      <section className='flex justify-center items-center '>
+        <Search />


The UI is a bit off. Please add the spacing around the search and filter.

Button should be in the same row as of search and filters

Neha · 2022-07-17T20:13:25Z

ui/src/Context/filter_context.js

+    dispatch({ type: UPDATE_FILTERS, payload: { name, value } });
+  }
+
+  function clearFilter() {


Are we removing only 1 filter at a time? If no, then the name should be clearFilters

renamed as per suggestion

Neha · 2022-07-17T21:12:00Z

ui/src/reducers/filter_reducer.js

+      }
+      if (status !== 'all') {
+        tempList = tempList.filter((value) => String(value.enabled) === status);
+        console.log(tempList, 'status', status);


Avoid having console.log

Neha · 2022-07-17T21:34:55Z

ui/src/Components/Search/Search.js

+        placeholder-opacity-25
+      '
+        name='userInput'
+        value={userInput}


please rename it to a self-explanatory name such as: searchValue or userSearchInput

renamed as per suggestion

Neha · 2022-07-17T21:40:34Z

ui/src/Components/Filters/Filters.js

+    updateFilters,
+  } = useFilterContext();
+
+  const handleValueChange = (event) => {


the name should be consistent with the rest of the event handlers. Instead of handleValueChange use onValueChange or something more align to the event happening.

renamed as per suggestion

Neha · 2022-07-17T21:45:47Z

ui/src/Components/Filters/Filters.js

+    ease-in-out
+    m-0
+    focus:text-gray-700 focus:bg-white focus:border-blue-600 focus:outline-none'
+        aria-label='.form-select-sm example'


aria-label is not correct here. Aria-label should be the label content

aria-label been removed as we are not using label.

Neha · 2022-07-17T21:46:39Z

ui/src/Components/Filters/Filters.js

+    m-0
+    focus:text-gray-700 focus:bg-white focus:border-blue-600 focus:outline-none'
+        aria-label='.form-select-sm example'
+        value={status}


The status is not implying the use/information about what information it would be holding. Please rename it

Neha · 2022-07-17T21:46:47Z

ui/src/Components/Filters/Filters.js

+    focus:text-gray-700 focus:bg-white focus:border-blue-600 focus:outline-none'
+        aria-label='.form-select-sm example'
+        value={status}
+        name='status'


The status is not implying the use/information about what information it would be holding. Please rename it

renamed to featureStatus as per suggestion

Neha · 2022-07-17T22:00:14Z

ui/src/App.js

+        <Search />
+        <Filters />
+      </section>
+      <button


button should be wrapped within the section of Filters and Search

button moved inside section

Neha · 2022-07-17T22:05:57Z

ui/src/Components/Filters/Filters.js

+  };
+
+  return (
+    <div className='mb-2 xl:w-96'>


use section tag instead of div for:

consistency

we use section for the section partition

renamed as per suggestion

pallabez · 2022-07-18T03:59:07Z

ui/src/App.js

+import { FEATURE_FLAGS, FEATURE_FLAGS_HEADERS } from './Mock/featureFlags';

 function App() {
+  const { filteredList, isLoading, featureFlagHeader, clearFilter } =


Should isLoading be a part of FilterContext?

It should be part of context where we are calling API

DeRaowl

Addressed all the review comments

DeRaowl · 2022-07-18T14:25:05Z

ui/src/App.js

-      />
+    <main className='flex flex-col items-center'>
+      <section className='flex justify-center items-center '>
+        <Search />


DeRaowl · 2022-07-18T14:25:42Z

ui/src/Components/Filters/Filters.js

+        name='status'
+        onChange={handleValueChange}
+      >
+        <option value='all'>Both</option>


Added dropdown

harshith-venkatesh · 2022-07-19T18:04:55Z

ui/src/reducers/FilterReducer.js

+            value.createdBy
+              .toLowerCase()
+              .includes(userSearchInput.toLowerCase()) ||
+            value.featureFlag
+              .toLowerCase()
+              .includes(userSearchInput.toLowerCase()) ||
+            value.repository
+              .toLowerCase()
+              .includes(userSearchInput.toLowerCase())


Is it good to create a function for the same?

Is it better to be present in util file? A function which could be used for search

harshith-venkatesh · 2022-07-19T18:05:19Z

ui/src/reducers/FilterReducer.js

+              .includes(userSearchInput.toLowerCase())
+        );
+      }
+      if (featureStatus !== 'all') {


Should 'all' be added to constants?

harshith-venkatesh · 2022-07-19T18:05:54Z

Good PR @DeRaowl , well written context, reducer

Neha · 2022-08-19T18:34:13Z

ui/src/Components/Filters/Filters.js

+        name='featureStatus'
+        onChange={onValueChange}
+      >
+        {FEATURE_STATUS.map((status, idx) => {


What is idx here?

Neha reviewed Jul 17, 2022

View reviewed changes

pallabez reviewed Jul 18, 2022

View reviewed changes

Neha mentioned this pull request Jul 18, 2022

[UI] Sort #33

Closed

DeRaowl commented Jul 18, 2022

View reviewed changes

DeRaowl force-pushed the feat/searchBar branch from b405298 to b8fa9f6 Compare July 18, 2022 14:46

harshith-venkatesh reviewed Jul 19, 2022

View reviewed changes

DeRaowl added 6 commits July 20, 2022 18:57

Resolved merge conflict by incorporating both suggestions.

3fdc84f

Resolved merge conflict by incorporating both suggestions.

8699fc3

Resolved merge conflict by incorporating both suggestions.

8751cae

Reuse created const file

7be75ff

Fix filename as per Pascal case

c25ae25

requested changes been implemented

fa932f2

DeRaowl force-pushed the feat/searchBar branch from ec5ca25 to fa932f2 Compare July 20, 2022 13:32

DeRaowl added 2 commits July 20, 2022 19:15

fix github check failure

c640b87

rename reducer folder as per Pascal case

8467cd3

Neha reviewed Aug 19, 2022

View reviewed changes

		@@ -0,0 +1,47 @@
		import { useState, useCallback } from 'react';

		@@ -0,0 +1,5 @@
		export const UPDATE_FILTERS = 'UPDATE_FILTERS';

Conversation

DeRaowl commented Jul 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

Neha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DeRaowl Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DeRaowl commented Jul 16, 2022 •

edited

Loading

DeRaowl Jul 18, 2022 •

edited

Loading