-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Update widgets.md (#1) #10
base: master
Are you sure you want to change the base?
Conversation
* Update widgets.md Signed-off-by: NxPKG <[email protected]> * Create Bored.vue Signed-off-by: NxPKG <[email protected]> * Update WidgetBase.vue Signed-off-by: NxPKG <[email protected]> --------- Signed-off-by: NxPKG <[email protected]>
Reviewer's Guide by SourceryThis pull request introduces a new "Bored" widget to the Shipyard project. The changes include updating the documentation, adding a new Vue component for the widget, and updating the WidgetBase component to include the new widget type. Class diagram for the new Bored widgetclassDiagram
class Bored {
+activity: Object
+fetchData()
+processData(data)
+endpoint: String
+type: String
+participants: String
+title: String
+metadata: Boolean
+price: String
+minprice: String
+maxprice: String
+accessibility: String
+minaccessibility: String
+maxaccessibility: String
}
class WidgetMixin
Bored --|> WidgetMixin
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @NxPKG - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using HTTPS instead of HTTP for the Bored API endpoint in the Bored.vue component for improved security.
- There's a small typo in the widgets.md file in the Bored widget's Info section: 'NOt' should be 'Not'.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 4 issues found
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
return ''; | ||
}, | ||
endpoint() { | ||
let url = `http://www.boredapi.com/api/activity?type=${this.type}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 suggestion (security): Consider using HTTPS for API requests to enhance security.
Using HTTP instead of HTTPS could expose the application to man-in-the-middle attacks. It's recommended to always use HTTPS for external API calls.
let url = `http://www.boredapi.com/api/activity?type=${this.type}`; | |
let url = `https://www.boredapi.com/api/activity?type=${this.type}`; |
type() { | ||
if (this.options.type !== undefined) { | ||
return this.options.type; | ||
} | ||
return ''; | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider refactoring computed properties to reduce code duplication.
Many computed properties follow the same pattern. Consider creating a helper function or using a more concise approach to handle these options, which would make the code more DRY (Don't Repeat Yourself).
type() { | |
if (this.options.type !== undefined) { | |
return this.options.type; | |
} | |
return ''; | |
}, | |
type() { | |
return this.options.type ?? ''; | |
}, |
.finally(() => { | ||
this.finishLoading(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): Consider moving finishLoading call to the then block for more accurate loading state.
Calling finishLoading in the finally block means it will be called even if there's an error. This could lead to confusion if the component appears loaded but actually encountered an error. Consider moving it to the then block.
.then((data) => {
// Process data here
this.finishLoading();
})
.catch((dataFetchError) => {
this.error('Unable to fetch any activities', dataFetchError);
});
#### Info | ||
|
||
- **CORS**: 🟢 Enabled | ||
- **Auth**: 🟢 NOt Required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (documentation): Fix typo in Auth field
Change 'NOt' to 'Not' for consistency and correctness.
**`minprice`** | `decimal` | _Optional_ | A factor describing the minimum cost of the activity with zero being free, and 1 being the most expensive. | ||
**`minprice`** | `decimal` | _Optional_ | A factor describing the maximum cost of the activity with zero being free, and 1 being the most expensive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (documentation): Correct duplicate 'minprice' entry
The second 'minprice' should be 'maxprice'. Please update this for accuracy.
```yaml | ||
- type: bored | ||
options: | ||
title: Would yo like to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick (documentation): Fix typo in example title
Change 'Would yo like to' to 'Would you like to'.
- type: bored | ||
options: | ||
title: Take a break | ||
metadata: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (documentation): Document or remove 'metadata' option
The 'metadata' option is used in an example but not listed in the options table. Either add it to the table or remove it from the example if it's not a valid option.
User description
Update widgets.md
Create Bored.vue
Update WidgetBase.vue
Category:
Overview
Issue Number (if applicable) #00
New Vars (if applicable)
Screenshot (if applicable)
Code Quality Checklist (Please complete)
PR Type
Enhancement, Documentation
Description
Bored
component that fetches and displays activity suggestions from the Bored API.WidgetBase.vue
to include thebored
widget in the compatibility mapping.Bored
widget, its options, examples, and API usage.Changes walkthrough 📝
Bored.vue
Add new Bored component for activity suggestions
src/components/Widgets/Bored.vue
Bored
component to fetch and display activities.WidgetBase.vue
Update WidgetBase for Bored component compatibility
src/components/Widgets/WidgetBase.vue
bored
to the compatibility mapping.widgets.md
Document new Bored widget with usage details
docs/widgets.md
Bored
widget.Summary by Sourcery
Add a new 'Bored' widget to the application, allowing users to receive activity suggestions from the Bored API. Update the documentation to reflect this new feature and provide usage examples.
New Features:
Documentation: