Update Search and Pagination on Assets Page & Edit Image Modal #249

matthugs · 2018-09-12T19:45:54Z

This updates the search and pagination components on the assets page (which are shared with the modal used to choose an image for an HTML unit) to use the paragon components provided for this purpose. Consistency!

fysheets

Overall I think this looks good -- I left some questions around things that I wanted to understand more clearly. I'm going to wait on the 👍until you squash and merge and it goes green.

src/components/AssetsSearch/AssetsSearch.test.jsx

src/components/AssetsSearch/index.jsx

fysheets · 2018-09-17T17:48:00Z

src/components/Pagination/Pagination.scss

@@ -29,3 +28,6 @@
    @extend .disabled;
  }
 }
+.page-item.disabled .page-link {
+  background-color: transparent;
+}


Can you share the context on why this CSS change was needed?

Yes; it's to override some CSS for making all the list items look similar. The disabled list items in this context are the ellipses representing ranges of pages not visible. The design of the paragon component is that those elements shouldn't have a distinct background color, and should blend in with the surrounding background color.

With that said I am quite bad at CSS/SCSS; is this the use-case for a SCSS variable override? It def looks like I could've put it above in the .disabled { ... .page-link { <here> } } above and achieved the same degree of specificity. I'll try that out for my next push.

src/components/Pagination/index.jsx

src/data/actions/assets.js

fysheets · 2018-09-18T15:38:37Z

Resolved conversations - thanks for the clarity. Two quick follow ups - please rebase on top of the latest master changes and a quick clarification on what the refactor commit message will do for version numbers.

matthugs · 2018-09-18T15:50:57Z

Resolved conversations - thanks for the clarity. Two quick follow ups - please rebase on top of the latest master changes and a quick clarification on what the refactor commit message will do for version numbers.

Good catch, @fysheets!

matthugs · 2018-09-18T15:54:58Z

package.json

@@ -15,7 +15,7 @@
  "license": "AGPL-3.0",
  "dependencies": {
    "@edx/edx-bootstrap": "^1.0.0",
-    "@edx/paragon": "3.1.2",
+    "@edx/paragon": "3.4.7",


I do have one final detail to follow-up on this PR which relates to some propType errors I expected, but didn't see. They were stemming from changes in edx/paragon I thought were necessary, but which we also ran into trouble trying to test locally. Following up on that in just a little bit! (It seems like the proptype validations went away when I bumped the paragon version, but my understanding was that I should still expect them even if we were up-to-date with @edx/paragon's master...

It looks like proptype validations aren't running against paragon components in the new version we're upgrading into. FYI @fysheets, since this may be a larger issue. We may have a number of proptype issues masked by this issue.

I haven't dug in super far, but I expect this may also be the result of how we use post-build paragon code from within studio-frontend.

We're moving forward from this with a bumped paragon version, which should suffice to eliminate any proptype validation errors.

codecov · 2018-09-19T20:53:04Z

Codecov Report

Merging #249 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #249      +/-   ##
==========================================
- Coverage   99.16%   99.15%   -0.01%     
==========================================
  Files          71       71              
  Lines        1312     1298      -14     
  Branches      222      220       -2     
==========================================
- Hits         1301     1287      -14     
  Misses         11       11

Impacted Files	Coverage Δ
src/components/Pagination/displayMessages.jsx	`100% <ø> (ø)`	⬆️
src/data/actions/assets.js	`100% <ø> (ø)`	⬆️
src/data/constants/actionTypes.js	`100% <ø> (ø)`	⬆️
src/data/reducers/assets.js	`98.78% <ø> (-0.02%)`	⬇️
src/components/AssetsSearch/displayMessages.jsx	`100% <ø> (ø)`	⬆️
src/components/AssetsSearch/index.jsx	`100% <100%> (ø)`	⬆️
src/components/Pagination/index.jsx	`100% <100%> (ø)`	⬆️

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 378219f...527bb5a. Read the comment docs.

fysheets

After running through this change locally - as we discussed, please add an action handler to the x / Clear Search button that appears once a search term is entered. Also, please rebase on top of master at some point. Thanks!

This exposes a port for node debugging in our docker container configuration, and documents how to make use of it in the README

docs(testDebugging): Document debugging tests within container

TravisBuddy · 2018-09-25T15:45:07Z

Continuous Integration Results

✅ Everything passed, go ahead and merge this once you're ready

fysheets · 2018-09-26T11:48:43Z

Sorry for the back and forth here - Thanks for updating that logic it looks awesome! I was running through it and Michael noticed that the CSS didn't quite match up to what may have been expected. We can sync up on that in person to determine the best course of action there. (Timebox to try to fix in this PR or a follow on task)

matthugs · 2018-09-26T12:54:25Z

Sorry for the back and forth here - Thanks for updating that logic it looks awesome! I was running through it and Michael noticed that the CSS didn't quite match up to what may have been expected. We can sync up on that in person to determine the best course of action there. (Timebox to try to fix in this PR or a follow on task)

No worries re:back-and-forth @fysheets and @MichaelRoytman! Happy to have close eyes on my first substantive changes

TravisBuddy · 2018-09-27T15:16:07Z

Continuous Integration Results

✅ Everything passed, go ahead and merge this once you're ready

src/components/Pagination/Pagination.scss

TravisBuddy · 2018-09-27T19:45:22Z

Continuous Integration Results

✅ Everything passed, go ahead and merge this once you're ready

fysheets

Thanks for tackling this even when it turned out to be a bigger task than anticipated!

Thumbing with the caveat of whatever the last scss decision ends up being within the team.

src/components/Pagination/index.jsx

Paragon's `Pagination` and `SearchField` components are now used for the Assets page and the parallel Add Image Modal. This involves some visual changes, and a difference in the number of pages ellided in the navigation bar. `perf` in the title here is used to force a new patch version, but `refactor` would likely be more descriptive. fixes EDUCATOR-3421

TravisBuddy · 2018-09-28T19:31:24Z

Continuous Integration Results

✅ Everything passed, go ahead and merge this once you're ready

matthugs requested a review from fysheets September 12, 2018 19:46

matthugs changed the title ~~[wip]: Update Search and Pagination on Assets Page & Edit Image Modal to~~ Update Search and Pagination on Assets Page & Edit Image Modal Sep 14, 2018

fysheets reviewed Sep 17, 2018

View reviewed changes

matthugs force-pushed the mhughes/assets-page-paragon-search branch from 4d1dfdc to ac082fa Compare September 17, 2018 18:52

matthugs force-pushed the mhughes/assets-page-paragon-search branch 2 times, most recently from 98be309 to 74b02e6 Compare September 18, 2018 15:50

matthugs commented Sep 18, 2018

View reviewed changes

matthugs force-pushed the mhughes/assets-page-paragon-search branch from 74b02e6 to c96e48d Compare September 19, 2018 20:53

fysheets reviewed Sep 24, 2018

View reviewed changes

matthugs force-pushed the mhughes/assets-page-paragon-search branch 2 times, most recently from 52c73aa to 3b54504 Compare September 25, 2018 13:39

Matt Hughes and others added 2 commits September 25, 2018 09:54

docs(tests): document debugging tests within container

7014379

This exposes a port for node debugging in our docker container configuration, and documents how to make use of it in the README

Merge pull request #252 from edx/matthugs/document-local-test-debugging

313acc3

docs(testDebugging): Document debugging tests within container

matthugs force-pushed the mhughes/assets-page-paragon-search branch 2 times, most recently from 2e6d2bb to 25baa9d Compare September 25, 2018 15:41

openedx deleted a comment from TravisBuddy Sep 25, 2018

matthugs force-pushed the mhughes/assets-page-paragon-search branch from 25baa9d to 6c8ac43 Compare September 27, 2018 15:12

fysheets reviewed Sep 27, 2018

View reviewed changes

src/components/Pagination/Pagination.scss Show resolved Hide resolved

matthugs force-pushed the mhughes/assets-page-paragon-search branch from 6c8ac43 to 50031ce Compare September 27, 2018 19:42

fysheets approved these changes Sep 28, 2018

View reviewed changes

dsjen reviewed Sep 28, 2018

View reviewed changes

src/components/Pagination/index.jsx Outdated Show resolved Hide resolved

MichaelRoytman and others added 2 commits September 28, 2018 15:20

remove show on hover for Update buttons

378219f

matthugs force-pushed the mhughes/assets-page-paragon-search branch from 50031ce to 527bb5a Compare September 28, 2018 19:27

MichaelRoytman force-pushed the master branch from 378219f to d5273a1 Compare September 28, 2018 19:56

matthugs merged commit 89b711e into master Sep 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Search and Pagination on Assets Page & Edit Image Modal #249

Update Search and Pagination on Assets Page & Edit Image Modal #249

matthugs commented Sep 12, 2018 •

edited

Loading

fysheets left a comment

fysheets Sep 17, 2018

matthugs Sep 17, 2018

fysheets commented Sep 18, 2018

matthugs commented Sep 18, 2018

matthugs Sep 18, 2018

matthugs Sep 18, 2018

matthugs Sep 24, 2018

codecov bot commented Sep 19, 2018 •

edited

Loading

fysheets left a comment

TravisBuddy commented Sep 25, 2018

fysheets commented Sep 26, 2018

matthugs commented Sep 26, 2018

TravisBuddy commented Sep 27, 2018

TravisBuddy commented Sep 27, 2018

fysheets left a comment

TravisBuddy commented Sep 28, 2018

Update Search and Pagination on Assets Page & Edit Image Modal #249

Update Search and Pagination on Assets Page & Edit Image Modal #249

Conversation

matthugs commented Sep 12, 2018 • edited Loading

fysheets left a comment

Choose a reason for hiding this comment

fysheets Sep 17, 2018

Choose a reason for hiding this comment

matthugs Sep 17, 2018

Choose a reason for hiding this comment

fysheets commented Sep 18, 2018

matthugs commented Sep 18, 2018

matthugs Sep 18, 2018

Choose a reason for hiding this comment

matthugs Sep 18, 2018

Choose a reason for hiding this comment

matthugs Sep 24, 2018

Choose a reason for hiding this comment

codecov bot commented Sep 19, 2018 • edited Loading

Codecov Report

fysheets left a comment

Choose a reason for hiding this comment

TravisBuddy commented Sep 25, 2018

Continuous Integration Results

fysheets commented Sep 26, 2018

matthugs commented Sep 26, 2018

TravisBuddy commented Sep 27, 2018

Continuous Integration Results

TravisBuddy commented Sep 27, 2018

Continuous Integration Results

fysheets left a comment

Choose a reason for hiding this comment

TravisBuddy commented Sep 28, 2018

Continuous Integration Results

matthugs commented Sep 12, 2018 •

edited

Loading

codecov bot commented Sep 19, 2018 •

edited

Loading