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

✨ Remove component repetition #758

Merged
merged 1 commit into from
Jun 24, 2022
Merged

✨ Remove component repetition #758

merged 1 commit into from
Jun 24, 2022

Conversation

patrickheeney
Copy link

@patrickheeney patrickheeney commented Jun 24, 2022

patrickheeney Medium patrickheeney /FEATURE/Improve-Widget-Base → Lissy93/dashy Commits: 1 | Files Changed: 1 | Additions: -439 Label

Category: Refactoring Only

Overview
My hope is to support specifying a custom widget type after mounting it in the components directory with docker. In order to support that the component needs to be specified in the config without mapping it to an internal known list of components. In this future you could do this:

# docker
volumes:
  - ./widgets/NewWidget.vue:/app/components/Widgets/NewWidget.vue

# config
- name: New Widget
  icon: fad fa-poll
  widgets:
    - type: NewWidget

# or maybe eventually define it in the config instead mapping individual files
customwidgets:
  - type: 'NewWidget
    source: '/app/components/CustomWidgets/NewWidget.vue'

These changes have the benefit of avoiding a lot of repetition and steps required to add new modules.

However the downside is that I needed to map the old type names to the actual component names. To avoid that mapping all of the components would need to be renamed from capital case to dash-lower-case. The alternative is to add churn and breaking changes by having users rename dash-lower-case to the actual component name. Its possible maybe some regex can remove and replace the dash with uppercase to have something that works both ways. There are a few exceptions however with iframe mapping to IframeWidget.

There is an eslint bug that I am working around by disabling the rule as well which I will detail below. I believe upping dependencies by fix it and remove the need for that.

Issue Number N/A

New Vars N/A

Screenshot N/A

Code Quality Checklist

  • All changes are backwards compatible
  • All lint checks and tests are passing
  • There are no (new) build warnings or errors
  • (If a new config option is added) Attribute is outlined in the schema and documented
  • (If a new dependency is added) Package is essential, and has been checked out for security or performance
  • Bumps version, if new feature added

@netlify
Copy link

netlify bot commented Jun 24, 2022

Deploy Preview for dashy-dev ready!

Name Link
🔨 Latest commit 4c87092
🔍 Latest deploy log https://app.netlify.com/sites/dashy-dev/deploys/62b52c07e2ea2f00085359cd
😎 Deploy Preview https://deploy-preview-758--dashy-dev.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@viezly
Copy link

viezly bot commented Jun 24, 2022

Changes preview:

Legend:

👀 Review pull request on Viezly

ErrorHandler('Widget type was not found');
return null;
}
// eslint-disable-next-line prefer-template
Copy link
Author

@patrickheeney patrickheeney Jun 24, 2022

Choose a reason for hiding this comment

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

This line and string concatenation is to work around eslint/eslint/issues/13394 and babel/babel/issues/10904. I haven't traced the dependencies yet to know what needs updated as that would require a separate PR.

Copy link
Owner

@Lissy93 Lissy93 left a comment

Choose a reason for hiding this comment

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

This is awesome, thank you :)
I don't know how I didn't think of this method!

return null;
}
// eslint-disable-next-line prefer-template
return () => import('@/components/Widgets/' + type + '.vue');
Copy link
Owner

@Lissy93 Lissy93 Jun 24, 2022

Choose a reason for hiding this comment

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

If the specified widget type isn't found, then we should show an error message. Since currently it will try and import this.widget.type if COMPAT[this.widgetType] is undefined.

Copy link
Author

Choose a reason for hiding this comment

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

I wasn't sure of the best method to do this. If you check the known COMPAT object, then that means no widgets can be loaded that are not on that map, eliminating the ability to pass a custom type. We could check the filesystem but that doesn't seem ideal either. As it stands now, the page will load, that section will appear empty and there is an error in the console.

Screen Shot 2022-06-24 at 7 34 56 AM

Thinking about it some more this morning, I'll catch the import error, log it to the console, and render the Blank widget instead. I should have another PR ready to go in a bit.

@Lissy93 Lissy93 merged commit a877ff2 into Lissy93:master Jun 24, 2022
@patrickheeney patrickheeney deleted the FEATURE/Improve-Widget-Base branch June 24, 2022 14:51
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.

2 participants