Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: applyCssClass value initialization #3182

Merged
merged 2 commits into from
Sep 9, 2020

Conversation

salarenko
Copy link
Contributor

Please provide a link to the associated issue.

Closes: #3181

Please provide a brief summary of this pull request.

This PR brings:

  • Proper initialization of _classMap[this._uuidv4] value, previously initialized with nested array instead of an array
  • Proper typing for updateComponentClassList(..., previousComponentClassList,..) argument

Please check whether the PR fulfills the following requirements

Documentation checklist:

@salarenko salarenko added bug Something isn't working core Core library specific issues labels Sep 1, 2020
@salarenko salarenko requested a review from a team September 1, 2020 12:27
@salarenko salarenko added this to In progress in Development via automation Sep 1, 2020
@salarenko salarenko self-assigned this Sep 1, 2020
@netlify
Copy link

netlify bot commented Sep 1, 2020

Deploy preview for fundamental-ngx ready!

Built with commit 045a65a

https://deploy-preview-3182--fundamental-ngx.netlify.app

@salarenko salarenko force-pushed the fix/3181-apply-css-class-decorator branch from 20218fb to e95e4a5 Compare September 1, 2020 12:46
@dimamarksman
Copy link
Contributor

would be good to add proper typing for firstCommonElementIndex as well

function firstCommonElementIndex(array1: string[], array2): number {

@dimamarksman
Copy link
Contributor

Looking at function signature and naming:

function firstCommonElementIndex(array1: string[], array2): number {
const index = array2.findIndex(element => array1.indexOf(element) !== -1);
return index === -1 ? 0 : index;
}

There are some things in this function that are not clear for me:

  • Why we try to find index in the second array and not first
  • Should this function be responsible for rounding index === -1 ? 0 : index

@salarenko
Copy link
Contributor Author

@dimamarksman

  • added argument typing for firstCommonElementIndex
  • I had similar concenrs to index "rounding", moved out of the function
  • you are right this should be arr1.(arr2) not arr2.(arr1)

I also added some logic to split multiple classes merged into one string
(['class1', 'class2 class3', 'class4'] => ['class1', 'class2', 'class3', 'class4'])

@InnaAtanasova InnaAtanasova changed the title Fix: applyCssClass value initialization fix: applyCssClass value initialization Sep 3, 2020
@InnaAtanasova
Copy link
Contributor

@dimamarksman let us know if you think the PR is ok (I'm not sure if you can approve it :) If you can, please approve)

@InnaAtanasova
Copy link
Contributor

LGTM

@dimamarksman
Copy link
Contributor

LGTM,
I'm not able to approve.

Development automation moved this from In progress to Reviewer approved Sep 8, 2020
@salarenko salarenko merged commit 064ab65 into master Sep 9, 2020
Development automation moved this from Reviewer approved to Done Sep 9, 2020
@salarenko salarenko deleted the fix/3181-apply-css-class-decorator branch September 9, 2020 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core Core library specific issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(core) Wrong assign in applyCssClass decorator
4 participants