De-dupe search bar components #30585

ajpal · 2019-09-04T17:53:34Z

Sound Library:
Before:

After:

Icon Library:
Before:

After:

Animation Library:
Before:

After:

joshlory · 2019-09-04T22:16:12Z

apps/src/templates/SearchBar.jsx

    position: 'relative'
  }
 };

 export default class AnimationPickerSearchBar extends React.Component {
  static propTypes = {
-    value: PropTypes.string.isRequired,
+    placeholderText: PropTypes.string.isRequired,
+    styles: PropTypes.object.isRequired,


This feels a little weird but I can't 100% put my finger on why. Allowing the consumer to style different elements in the component makes sense, and so does combining all those properties into one object (I think). Maybe we should be explicit about what keys it takes:

Suggested change

styles: PropTypes.object.isRequired,

styles: PropTypes.object.shape({

searchArea: PropTypes.object,

icon: PropTypes.object,

input: PropTypes.object

});

What do you think?

oo, didn't know you could specify what keys the object has! Yeah that definitely feels better, thanks!

Do all of these styles need to be customized? Or can we make smart defaults, and have them be the same everywhere <SearchBar/> gets used?

Co-Authored-By: Josh Lory <josh.lory@code.org>

joshlory · 2019-09-04T22:24:14Z

apps/src/templates/SearchBar.jsx

@@ -31,32 +31,37 @@ const styles = {
    top: 9,
    left: 10
  },
-  search: {
+  searchArea: {
    position: 'relative'
  }
 };

 export default class AnimationPickerSearchBar extends React.Component {


Nit: SearchBar?

apps/src/templates/SearchBar.jsx

…rg/code-dot-org into sep3-search-component

codecov-io · 2019-09-05T00:48:58Z

Codecov Report

Merging #30585 into staging will decrease coverage by 1.16%.
The diff coverage is 80%.

@@             Coverage Diff             @@
##           staging   #30585      +/-   ##
===========================================
- Coverage    73.22%   72.05%   -1.17%     
===========================================
  Files         2059     1379     -680     
  Lines       113161    85225   -27936     
  Branches      3499     3496       -3     
===========================================
- Hits         82864    61412   -21452     
+ Misses       27006    20546    -6460     
+ Partials      3291     3267      -24

Flag	Coverage Δ
#integration	`55.75% <68.42%> (ø)`	⬆️
#storybook	`56.52% <73.52%> (-0.01%)`	⬇️
#unit	`58.09% <78.94%> (-0.03%)`	⬇️

Impacted Files	Coverage Δ
apps/src/code-studio/components/IconLibrary.jsx	`82.14% <0%> (ø)`	⬆️
.../src/p5lab/AnimationPicker/AnimationPickerBody.jsx	`72.61% <100%> (+0.11%)`	⬆️
apps/src/code-studio/components/SoundLibrary.jsx	`81.81% <66.66%> (+1.42%)`	⬆️
apps/src/templates/SearchBar.jsx	`83.87% <77.27%> (ø)`
apps/src/templates/SearchBar.story.jsx	`90% <85.71%> (ø)`
...emplates/manageStudents/ManageStudentsNameCell.jsx	`85.41% <0%> (-4.17%)`	⬇️
...lates/manageStudents/ManageStudentsActionsCell.jsx	`63.29% <0%> (-2.54%)`	⬇️
.../application_dashboard/admin_cohort_view_table.jsx	`37.5% <0%> (-1.98%)`	⬇️
...udio/pd/application_dashboard/quick_view_table.jsx	`33.68% <0%> (-1.67%)`	⬇️
...d/workshop_dashboard/components/workshop_table.jsx	`32.43% <0%> (-1.49%)`	⬇️
... and 709 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1906b92...deed22e. Read the comment docs.

codecov-io · 2019-09-05T00:49:35Z

Codecov Report

Merging #30585 into staging will decrease coverage by 1.16%.
The diff coverage is 80%.

@@             Coverage Diff             @@
##           staging   #30585      +/-   ##
===========================================
- Coverage    73.22%   72.05%   -1.17%     
===========================================
  Files         2059     1379     -680     
  Lines       113161    85225   -27936     
  Branches      3499     3496       -3     
===========================================
- Hits         82864    61412   -21452     
+ Misses       27006    20546    -6460     
+ Partials      3291     3267      -24

Flag	Coverage Δ
#integration	`55.75% <68.42%> (ø)`	⬆️
#storybook	`56.52% <73.52%> (-0.01%)`	⬇️
#unit	`58.09% <78.94%> (-0.03%)`	⬇️

Impacted Files	Coverage Δ
apps/src/code-studio/components/IconLibrary.jsx	`82.14% <0%> (ø)`	⬆️
.../src/p5lab/AnimationPicker/AnimationPickerBody.jsx	`72.61% <100%> (+0.11%)`	⬆️
apps/src/code-studio/components/SoundLibrary.jsx	`81.81% <66.66%> (+1.42%)`	⬆️
apps/src/templates/SearchBar.jsx	`83.87% <77.27%> (ø)`
apps/src/templates/SearchBar.story.jsx	`90% <85.71%> (ø)`
...emplates/manageStudents/ManageStudentsNameCell.jsx	`85.41% <0%> (-4.17%)`	⬇️
...lates/manageStudents/ManageStudentsActionsCell.jsx	`63.29% <0%> (-2.54%)`	⬇️
.../application_dashboard/admin_cohort_view_table.jsx	`37.5% <0%> (-1.98%)`	⬇️
...udio/pd/application_dashboard/quick_view_table.jsx	`33.68% <0%> (-1.67%)`	⬇️
...d/workshop_dashboard/components/workshop_table.jsx	`32.43% <0%> (-1.49%)`	⬇️
... and 709 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1906b92...deed22e. Read the comment docs.

codecov-io · 2019-09-05T00:50:09Z

Codecov Report

Merging #30585 into staging will decrease coverage by 1.16%.
The diff coverage is 80.95%.

@@             Coverage Diff             @@
##           staging   #30585      +/-   ##
===========================================
- Coverage    73.22%   72.05%   -1.17%     
===========================================
  Files         2059     1379     -680     
  Lines       113161    85223   -27938     
  Branches      3499     3495       -4     
===========================================
- Hits         82864    61410   -21454     
+ Misses       27006    20547    -6459     
+ Partials      3291     3266      -25

Flag	Coverage Δ
#integration	`55.75% <71.42%> (ø)`	⬆️
#storybook	`56.52% <76.47%> (-0.01%)`	⬇️
#unit	`58.09% <78.57%> (-0.03%)`	⬇️

Impacted Files	Coverage Δ
apps/src/code-studio/components/IconLibrary.jsx	`82.14% <0%> (ø)`	⬆️
apps/src/templates/SearchBar.jsx	`86.2% <100%> (ø)`
.../src/p5lab/AnimationPicker/AnimationPickerBody.jsx	`72.61% <100%> (+0.11%)`	⬆️
apps/src/code-studio/components/SoundLibrary.jsx	`81.81% <66.66%> (+1.42%)`	⬆️
apps/src/templates/SearchBar.story.jsx	`90% <85.71%> (ø)`
...emplates/manageStudents/ManageStudentsNameCell.jsx	`85.41% <0%> (-4.17%)`	⬇️
...lates/manageStudents/ManageStudentsActionsCell.jsx	`63.29% <0%> (-2.54%)`	⬇️
.../application_dashboard/admin_cohort_view_table.jsx	`37.5% <0%> (-1.98%)`	⬇️
...udio/pd/application_dashboard/quick_view_table.jsx	`33.68% <0%> (-1.67%)`	⬇️
...d/workshop_dashboard/components/workshop_table.jsx	`32.43% <0%> (-1.49%)`	⬇️
... and 710 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1906b92...44d6ee4. Read the comment docs.

joshlory · 2019-09-06T16:27:11Z

apps/src/templates/SearchBar.jsx

+  render() {
+    let customStyles = this.props.styles || {};
+    return (
+      <div style={{...styles.searchArea, ...customStyles.searchArea}}>


Are these style overrides still needed? It doesn't look like any consumer is passing these props in.

joshlory

Nice cleanup!

ajpal added 2 commits September 4, 2019 10:35

search component

dd182f1

Add search bar to storybook

f2b787d

ajpal requested a review from joshlory September 4, 2019 17:53

i18n placeholder text, delete AniamtionPickerSearchBar

94406a5

joshlory reviewed Sep 4, 2019

View reviewed changes

Update apps/src/templates/SearchBar.jsx

aa26f13

Co-Authored-By: Josh Lory <josh.lory@code.org>

joshlory reviewed Sep 4, 2019

View reviewed changes

apps/src/templates/SearchBar.jsx Outdated Show resolved Hide resolved

joshlory and others added 3 commits September 4, 2019 16:08

Amend aa26f13

16d1e29

better default styles

57c49e4

Merge branch 'sep3-search-component' of https://github.com/code-dot-o…

deed22e

…rg/code-dot-org into sep3-search-component

ajpal added 2 commits September 5, 2019 14:57

[wip] width to fit container

5f5186c

better default styles

ee0c07b

joshlory reviewed Sep 6, 2019

View reviewed changes

joshlory approved these changes Sep 6, 2019

View reviewed changes

remove custom styles prop

44d6ee4

ajpal merged commit 733b12e into staging Sep 6, 2019

ajpal deleted the sep3-search-component branch September 6, 2019 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

De-dupe search bar components #30585

De-dupe search bar components #30585

ajpal commented Sep 4, 2019 •

edited

joshlory Sep 4, 2019

ajpal Sep 4, 2019

joshlory Sep 4, 2019

joshlory Sep 4, 2019

codecov-io commented Sep 5, 2019 •

edited

codecov-io commented Sep 5, 2019

codecov-io commented Sep 5, 2019 •

edited

joshlory Sep 6, 2019

joshlory left a comment

-    styles: PropTypes.object.isRequired,
+    styles: PropTypes.object.shape({
+      searchArea: PropTypes.object,
+      icon: PropTypes.object,
+      input: PropTypes.object
+    });

De-dupe search bar components #30585

De-dupe search bar components #30585

Conversation

ajpal commented Sep 4, 2019 • edited

joshlory Sep 4, 2019

Choose a reason for hiding this comment

ajpal Sep 4, 2019

Choose a reason for hiding this comment

joshlory Sep 4, 2019

Choose a reason for hiding this comment

joshlory Sep 4, 2019

Choose a reason for hiding this comment

codecov-io commented Sep 5, 2019 • edited

Codecov Report

codecov-io commented Sep 5, 2019

Codecov Report

codecov-io commented Sep 5, 2019 • edited

Codecov Report

joshlory Sep 6, 2019

Choose a reason for hiding this comment

joshlory left a comment

Choose a reason for hiding this comment

ajpal commented Sep 4, 2019 •

edited

codecov-io commented Sep 5, 2019 •

edited

codecov-io commented Sep 5, 2019 •

edited