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

Refactoring for includePanelProcessor and MdAttributeRenderer #1817

Closed
jovyntls opened this issue Mar 11, 2022 · 0 comments · Fixed by #1896
Closed

Refactoring for includePanelProcessor and MdAttributeRenderer #1817

jovyntls opened this issue Mar 11, 2022 · 0 comments · Fixed by #1896

Comments

@jovyntls
Copy link
Contributor

Is your request related to a problem?
These classes could use some refactoring (details below) following #1780 where a few helper functions were created but have not been implemented throughout. The code review for this PR pointed out some improvements that could be made for related methods as well.

Describe the solution you'd like

Suggestions MdAttributeRenderer:

  • attributeSlotElement in processAttributeWithoutOverride can use the createSlotTemplateNode function in elements.js
  • hasSlotOverridingAttribute can be used in other processor functions, e.g. processDropdownAttributes, to reduce code duplication

Suggestions includePanelProcessor:
In the code review for popovers, @ryoarmanda pointed out several issues that could be applied to processInclude and processPanelSrc as well (details can be found here):

  • Early returns in places where fall-through behaviour is undesirable, e.g.
    • in processInclude, if the src attribute is found to be empty
    • in processInclude, if the maximum callstack size is exceeded
  • Consistency in the returned values: the caller of processNode expects a Context object to be returned
    • in processInclude, return node can be changed to return context
    • same for processPanelSrc
  • in processInclude, the if (isUrl) { ... } block is never reached as _getFileExistsNode above will catch any URLs first. Hence, the URL check can be moved up to before _getFileExistsNode.

Describe alternatives you've considered

N/A

Additional context

N/A

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

Successfully merging a pull request may close this issue.

2 participants