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: allow componentConfig props to properly merge with component props #416

Closed
wants to merge 1 commit into from

Conversation

joshhowenstine
Copy link
Contributor

@joshhowenstine joshhowenstine commented Nov 20, 2023

Description

Props set in componentConfig are not properly merged and are leaving some style properties when themes are changed at runtime. Also any componentConfig properties should deep merge with properties that are set at the instance level.

References

Ticket

Testing

  1. Open the Tile component
  2. Right click on the iframe window containing the component in storybook and open the webmaster tools
  3. Observe that no image scale on artwork is present on focus.
  4. window.CONTEXT.setTheme({componentConfig:{Tile:{artwork:{style:{imageScale:5,},},style:{paddingYProgress:10,paddingYBetweenContent:10,},},},});
  5. After update when focused you should see an imageScale effect when focusing on the Tile
  6. Any combination not including the scale property should clear it. window.CONTEXT.setTheme({componentConfig:{Tile:{artwork:{},style:{paddingYProgress:10,paddingYBetweenContent:10,},},},});
  7. Setting any value on an instance of the component itself should not allow the componentConfig to overwrite it.

image

Automation

Checklist

  • all commented code has been removed
  • any new console issues have been resolved
  • code linter and formatter has been run
  • test coverage meets repo requirements
  • PR name matches the expected semantic-commit syntax

@rdkcmf-jenkins
Copy link

Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

  • Protex Server Path: /home/blackduck/github/Lightning-UI-Components/416/rdkcentral/Lightning-UI-Components

  • Commit: 94c51dd

Report detail: gist

@svc-lightning-ui-components
Copy link
Collaborator

Test Execution Passed.

@svc-lightning-ui-components
Copy link
Collaborator

Test Execution Failed.

@rdkcmf-jenkins
Copy link

Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

  • Protex Server Path: /home/blackduck/github/Lightning-UI-Components/416/rdkcentral/Lightning-UI-Components

  • Commit: 94c51dd

Report detail: gist

@rdkcmf-jenkins
Copy link

WARNING: A Blackduck scan failure has been waived

A prior failure has been upvoted

  • Upvote reason: existing code, not this PR - will chase separately

  • Commit: 94c51dd

@arwehrman
Copy link
Contributor

I created a build and pulled it into IS. Everything looks ok but there are a few tests failing. Looks like there all variations of Rail so not sure if this related to your changes or not.

Copy link
Contributor

@erautenberg erautenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran a yarn build and copied the dist over to IS LUI and the imageScale doesn't seem to be applied correctly at all anymore, no matter the theme. In the Tile story in the Xfinity theme, nothing seems to happen when swapping between focus and unfocus when it should be scaling. On the Rail story, if I start on the Xfinity theme, it seems like the first tile in the rail is stuck in the scaled up view, and if I switch to Mosaic, it is still stuck that way as well.

Update: The imageScale in current IS LUI seems totally broken already and is not updating on focus/unfocus. Maybe this was a result of the last update?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants