Adding proposed theme with new themeable dropdowns #43

hutchgrant · 2018-12-06T16:10:28Z

Related Issue

Inspired by PR #42 @DevLab2425 I took it a step further and wrote my own themeable custom dropdown component.

Summary of Changes

changed page background color
added themeable vars for table/headers/dropdowns
added new themeable dropdowns
modified home page dropdown automatically manages event target values/indexes

thescientist13

@hutchgrant
Very cool, and nice work! 🎨

Couple observations:

Text looks like squished with the dropdown arrow, not sure if the dropdown needs a dynamic width?

Code coverage looks like it dropped. Anyway to bring it back up?

thescientist13 · 2019-01-01T20:21:21Z

src/components/dropdown/dropdown.js

+    this.dropdown.scrollTop = 0;
+  }
+
+  handleDropdown(drop) {


A couple name changes here might improve the readability of the code a bit more:

Consider calling it setSelectedDropdownOption?

It might be a bit more explicit to call it selectedOption, as opposed to drop

thescientist13 · 2019-01-01T20:34:08Z

src/components/dropdown/dropdown.js

+  }
+
+  connectedCallback() {
+    this.shadowRoot.addEventListener('click', this.handleEvent.bind(this), true);


would we want consider calling this handleDropdownClickEvent?

hutchgrant · 2019-01-01T21:33:53Z

src/components/dropdown/dropdown.css

+      & input:checked + label {
+        display:block;
+        border-top: none;
+        position: absolute;


That's a nasty bug I didn't notice before. The problem is when an item is selected the selected label changes to position:absolute and set to top:0 that's why we see it as the selected option once the dropdown disappears. But once the absolute position is set, the previous width, which accounted for that label's width, disappears, and now, the next largest item is used to calculate the width(or it defaults to a minimum width of 12em).

I suppose one way to fix it would be to do change the selected option's label programmatically on click event with javascript instead of using css to position its label at the top.

thescientist13 · 2019-01-16T00:44:06Z

Interesting, I get mixed unit testing results locally, sometimes 90%, sometimes 86% 🤔

HeadlessChrome 0.0.0 (Mac OS X 10.14.1): Executed 27 of 27 SUCCESS (0.046 secs / 0.053 secs)
TOTAL: 27 SUCCESS
TOTAL: 27 SUCCESS

=============================== Coverage summary ===============================
Statements   : 100% ( 59/59 )
Branches     : 90.91% ( 20/22 )
Functions    : 100% ( 20/20 )
Lines        : 100% ( 58/58 )
================================================================================
^C
Owens-MacBook-Pro-2:www.contributary.community owenbuckley$ yarn test
yarn run v1.12.3
$ yarn clean && karma start
$ rimraf ./public ./reports
(node:38088) DeprecationWarning: Tapable.plugin is deprecated. Use new API on `.hooks` instead
ℹ ｢wdm｣: wait until bundle finished: noop
15 01 2019 19:42:22.716:WARN [karma]: No captured browser, open http://localhost:9876/
⚠ ｢wdm｣: Hash: cac6c053094b624c9e8b
Version: webpack 4.23.1
Time: 12352ms
Built at: 01/15/2019 7:42:22 PM
                                         Asset      Size           Chunks             Chunk Names
          36fa4d065dc847667294c72f71ef7758.png  7.69 KiB                   [emitted]
          3d74e8bfd4ef7985f7529bb9f7650eca.png  11.1 KiB                   [emitted]
          c56e47aa6659c9c36cbd3bc6c5c0b137.png   115 KiB                   [emitted]
          index.2a8aef33ad629390d43f.bundle.js   460 KiB            index  [emitted]  index
                                    index.html   1.8 KiB                   [emitted]
karma-test-shim.0f5595b6d10d09bc31f4.bundle.js   425 KiB  karma-test-shim  [emitted]  karma-test-shim
Entrypoint index = index.2a8aef33ad629390d43f.bundle.js
Entrypoint karma-test-shim = karma-test-shim.0f5595b6d10d09bc31f4.bundle.js
[../karma-test-shim.js] ./karma-test-shim.js 291 bytes {karma-test-shim} [built]
[../node_modules/@polymer/lit-element/lib/decorators.js] ./node_modules/@polymer/lit-element/lib/decorators.js 3.55 KiB {index} {karma-test-shim} [built]
[../node_modules/@polymer/lit-element/lib/updating-element.js] ./node_modules/@polymer/lit-element/lib/updating-element.js 18.3 KiB {index} {karma-test-shim} [built]
[../node_modules/@polymer/lit-element/lit-element.js] ./node_modules/@polymer/lit-element/lit-element.js 2 KiB {index} {karma-test-shim} [built]
[./ sync recursive \.spec\.js$] ./src sync \.spec\.js$ 330 bytes {karma-test-shim} [built]
[./app/app.css] ./src/app/app.css 421 bytes {index}
[./app/app.js] ./src/app/app.js 6.47 KiB {index} [built]
[./components/dropdown/dropdown.spec.js] ./src/components/dropdown/dropdown.spec.js 3.67 KiB {karma-test-shim} [optional] [built] [1 warning]
[./components/footer/footer.js] ./src/components/footer/footer.js 7.01 KiB {index} {karma-test-shim} [built]
[./components/footer/footer.spec.js] ./src/components/footer/footer.spec.js 1.91 KiB {karma-test-shim} [optional] [built]
[./components/header/header.js] ./src/components/header/header.js 6.27 KiB {index} {karma-test-shim} [built]
[./components/header/header.spec.js] ./src/components/header/header.spec.js 582 bytes {karma-test-shim} [optional] [built]
[./components/issues-list/issues-list.spec.js] ./src/components/issues-list/issues-list.spec.js 4.4 KiB {karma-test-shim} [optional] [built]
[./index.js] ./src/index.js 887 bytes {index} [built]
[./pages/home/home.js] ./src/pages/home/home.js 29.7 KiB {index} [built]
    + 32 hidden modules

WARNING in ./src/components/dropdown/dropdown.spec.js
Module Warning (from ../node_modules/eslint-loader/index.js):

/Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/src/components/dropdown/dropdown.spec.js
  21:7  warning  Unexpected console statement  no-console
  22:7  warning  Unexpected console statement  no-console

✖ 2 problems (0 errors, 2 warnings)

 @ ./src sync \.spec\.js$ ./components/dropdown/dropdown.spec.js
 @ ./karma-test-shim.js
Child html-webpack-plugin for "index.html":
         Asset     Size  Chunks  Chunk Names
    index.html  535 KiB       1
    Entrypoint undefined = index.html
    [../node_modules/html-webpack-plugin/lib/loader.js!./index.html] ./node_modules/html-webpack-plugin/lib/loader.js!./src/index.html 1.76 KiB {1} [built]
    [../node_modules/lodash/lodash.js] ./node_modules/lodash/lodash.js 527 KiB {1} [built]
    [../node_modules/webpack/buildin/global.js] (webpack)/buildin/global.js 492 bytes {1} [built]
    [../node_modules/webpack/buildin/module.js] (webpack)/buildin/module.js 552 bytes {1} [built]
ℹ ｢wdm｣: Compiled with warnings.
15 01 2019 19:42:22.729:INFO [karma-server]: Karma v3.1.1 server started at http://0.0.0.0:9876/
15 01 2019 19:42:22.730:INFO [launcher]: Launching browsers ChromiumHeadlessConfigured with concurrency unlimited
15 01 2019 19:42:22.749:INFO [launcher]: Starting browser ChromeHeadless
15 01 2019 19:42:23.120:INFO [HeadlessChrome 0.0.0 (Mac OS X 10.14.1)]: Connected on socket Xt4RmlvkM2Z3xzh3AAAA with id 13818903
LOG: 'selected', 'value0'
HeadlessChrome 0.0.0 (Mac OS X 10.14.1): Executed 0 of 27 SUCCESS (0 secs / 0 secs)
LOG: 'selected', 'value0'
LOG: 'index', 0
HeadlessChrome 0.0.0 (Mac OS X 10.14.1): Executed 0 of 27 SUCCESS (0 secs / 0 secs)
LOG: 'index', 0
LOG: 'selected', 'value0'
HeadlessChrome 0.0.0 (Mac OS X 10.14.1): Executed 0 of 27 SUCCESS (0 secs / 0 secs)
LOG: 'selected', 'value0'
LOG: 'index', 0
HeadlessChrome 0.0.0 (Mac OS X 10.14.1): Executed 0 of 27 SUCCESS (0 secs / 0 secs)
LOG: 'index', 0
 HeadlessChrome 0.0.0 (Mac OS X 10.14.1): Executed 27 of 27 SUCCESS (0.051 secs / 0.056 secs)
.
HeadlessChrome 0.0.0 (Mac OS X 10.14.1): Executed 27 of 27 SUCCESS (0.051 secs / 0.056 secs)
TOTAL: 27 SUCCESS
TOTAL: 27 SUCCESS

=============================== Coverage summary ===============================
Statements   : 100% ( 59/59 )
Branches     : 86.36% ( 19/22 )
Functions    : 100% ( 20/20 )
Lines        : 100% ( 58/58 )
================================================================================

Gonna update it to 85% just to make sure the fluctuation doesn't bottleneck our CI here, but definitely something that might worth following up on in the Instanbul repo maybe.

thescientist13

Looks great, thanks so much @hutchgrant !

hutchgrant added 4 commits December 6, 2018 11:05

Adding propose theme with new themeable dropdowns

5a6871a

Fixing checkbox boolean bind, refactoring

3aa8022

Refactoring, removing redundancies

c50c33f

Fixing missing event removal on disconnect

e67afcd

thescientist13 requested review from thescientist13 and DevLab2425 January 1, 2019 20:18

thescientist13 reviewed Jan 1, 2019

View reviewed changes

thescientist13 mentioned this pull request Jan 1, 2019

Style dropdowns #42

Closed

hutchgrant commented Jan 1, 2019

View reviewed changes

hutchgrant added 2 commits January 1, 2019 17:03

Fixing dynamic width of selected option bug

ba0f19d

Fixing test coverage

9def960

hutchgrant force-pushed the style-proposal branch from 643d943 to 9def960 Compare January 7, 2019 07:01

update threshold

fee2f70

thescientist13 approved these changes Jan 16, 2019

View reviewed changes

thescientist13 merged commit 8abd3ec into develop Jan 16, 2019

thescientist13 deleted the style-proposal branch January 16, 2019 00:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding proposed theme with new themeable dropdowns #43

Adding proposed theme with new themeable dropdowns #43

hutchgrant commented Dec 6, 2018

thescientist13 left a comment

thescientist13 Jan 1, 2019

thescientist13 Jan 1, 2019

hutchgrant Jan 1, 2019

thescientist13 commented Jan 16, 2019

thescientist13 left a comment

Adding proposed theme with new themeable dropdowns #43

Adding proposed theme with new themeable dropdowns #43

Conversation

hutchgrant commented Dec 6, 2018

Related Issue

Summary of Changes

thescientist13 left a comment

Choose a reason for hiding this comment

thescientist13 Jan 1, 2019

Choose a reason for hiding this comment

thescientist13 Jan 1, 2019

Choose a reason for hiding this comment

hutchgrant Jan 1, 2019

Choose a reason for hiding this comment

thescientist13 commented Jan 16, 2019

thescientist13 left a comment

Choose a reason for hiding this comment