Datepicker bugfixes + theme support #35
Conversation
package.json
Outdated
| @@ -87,6 +85,10 @@ | |||
| "tiny-lr": "^1.1.1", | |||
| "vinyl-source-stream": "^2.0.0" | |||
| }, | |||
| "dependencies": { | |||
| "nebenan-react-datepicker": "^1.0.0", | |||
| "nebenan-helpers": "^4.1.1" | |||
There was a problem hiding this comment.
-> peerDependencies
do we even need it?
There was a problem hiding this comment.
nebenan-helpers is used in datepicker (dom, keymanager, eventproxy).
Will add them to peerDependencies - but I don't fully get why. It's not like they are some kind of plugins. Wouldn't matter to have two different versions in dependency tree. (npm flattens dependencies by itself as far as I know, so as long it's not nebenan-react-datepicker ^2.x.x, every ^1.x.x should resolve to the same package)
peterhass
force-pushed
the
fix-datepicker-dependency
branch
from
9f9219f to
4ad6182
Compare
| @@ -1,2 +1,2 @@ | |||
| @import 'nebenan-ui-kit/_index.scss'; | |||
| @import 'src/**/*.scss'; | |||
| @import 'src/**/index.scss'; | |||
There was a problem hiding this comment.
Preview in nebenan-form loaded datepickers theme.scss, nebenan-frontend didn't import it because it only imports index.scss files.
Note: All other scss files in nebenan-form/src/** are called index.scss.
package.json
Outdated
| @@ -84,12 +82,16 @@ | |||
| "stylelint": "^13.2.1", | |||
| "stylelint-config-nebenan": "^1.6.0", | |||
| "terser": "^4.6.7", | |||
| "nebenan-react-datepicker": "^1.0.0", | |||
There was a problem hiding this comment.
nebenan-react-datepicker needs to be a direct dependency
nebenan-helpers needs to be a peer (loaded by multiple packages, need to insure single version)
src/datepicker/utils.es
Outdated
|
|
||
| const stripPrefix = (key, prefix) => { | ||
| const strippedKey = key.substring(prefix.length); | ||
| return strippedKey.charAt(0).toLowerCase() + strippedKey.slice(1); |
There was a problem hiding this comment.
key.replace(new RegExp(^${prefix}), '') ?
precompile regex out of the loop and you won't need the function?
There was a problem hiding this comment.
Regex may even be slower and I would still need to convert first character to lower case
calendarRoot -> root
calendarSuperPicker -> superPicker
src/datepicker/utils.es
Outdated
| return strippedKey.charAt(0).toLowerCase() + strippedKey.slice(1); | ||
| }; | ||
|
|
||
| export const getSubTheme = (theme, prefix) => ( |
There was a problem hiding this comment.
theme = {} would reduce code
src/datepicker/utils.es
Outdated
|
|
||
| export const getSubTheme = (theme, prefix) => ( | ||
| Object.entries(theme || {}) | ||
| .reduce((acc, [key, value]) => { |
There was a problem hiding this comment.
you seem to double check the key:
1 .startsWith
2 .substring
just generate 1 regex with prefix and run over all, if there aint no prefix it will just do nothing
There was a problem hiding this comment.
Won't work because ...
- getSubTheme should only return the prefixed keys, not the others
- Still need to replace first char after prefix see other comment Datepicker bugfixes + theme support #35 (comment)
- stripPrefix has no additional check. The only one is in getSubTheme
src/datepicker/utils.es
Outdated
| if (!otherTheme) return baseTheme; | ||
|
|
||
| const result = {}; | ||
| Object.keys(baseTheme).forEach((key) => { |
There was a problem hiding this comment.
if return an object why not reduce?
src/datepicker/utils.es
Outdated
| const baseValue = baseTheme[key]; | ||
| const extendValue = otherTheme[key]; | ||
|
|
||
| result[key] = [baseValue, extendValue].filter((v) => Boolean(v)).join(' '); |
There was a problem hiding this comment.
could use cslx or write so [baseValue, extendValue].filter(Boolean).join(' ')
|
@suprMax ready for next run |
| @@ -23,6 +24,7 @@ class Datepicker extends InputComponent { | |||
| 'handleClick', | |||
| 'handleClear', | |||
| ); | |||
| this.getCalendarTheme = memorize(getCalendarTheme.bind(null, baseCalendarTheme)); | |||
There was a problem hiding this comment.
lodash memorize only uses first argument as cache key. Good thing I read the documentation twice.
package.json
Outdated
| "peerDependencies": { | ||
| "@babel/runtime": "^7.9.2", | ||
| "clsx": "^1.1.0", | ||
| "nebenan-helpers": "^4.2.0-beta.0", | ||
| "tiny-lr": "^1.1.1", |
Datepicker in 8.3.0 was missing nebenan-react-datepicker package (ÜBERRASCHUNG).