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

chore: code cleanup #225

Merged
merged 11 commits into from
May 20, 2024
Merged

chore: code cleanup #225

merged 11 commits into from
May 20, 2024

Conversation

miquelbeltran
Copy link
Contributor

@miquelbeltran miquelbeltran commented May 17, 2024

chore: code cleanup

Description 📝

  • Purpose: Multiple small code cleanups.
  • Approach: Different commits for small stuff that came in the late review of feat: Breadcrumbs #210

Type of change

  • Chore

Updates

Test plan 🧪

  • Run example. CI jobs should pass as well. Changes are mostly cosmetic.

Author to check 👓

  • Project and all contained modules builds successfully
  • Self-/dev-tested
  • Unit/UI/Automation/Integration tests provided where applicable
  • Code is written to standards
  • Appropriate documentation written (code comments, internal docs)

Reviewer to check ✔️

  • Project and all contained modules builds successfully
  • Change has been dev-/reviewer-tested, where possible
  • Unit/UI/Automation/Integration tests provided where applicable
  • Code is written to standards
  • Appropriate documentation written (code comments, internal docs)

@miquelbeltran miquelbeltran marked this pull request as ready for review May 17, 2024 06:36
@miquelbeltran miquelbeltran requested review from a team, TheRealAgentK, PanosNB and sumitramanga and removed request for a team May 17, 2024 06:36
@miquelbeltran miquelbeltran mentioned this pull request May 17, 2024
13 tasks
Copy link
Contributor

@TheRealAgentK TheRealAgentK left a comment

Choose a reason for hiding this comment

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

LGTM

@TheRealAgentK
Copy link
Contributor

Ok, so this has a bunch of conflicts, leaving to @miquelbeltran to have a look at resolving before merge.

@@ -23,8 +24,7 @@ app.set("view engine", "ejs");
// Add the Raygun breadcrumb Express handler
app.use(raygunClient.expressHandlerBreadcrumbs);

// uncomment after placing your favicon in /public
// app.use(favicon(__dirname + '/public/favicon.ico'));
app.use(favicon(__dirname + "/public/favicon.ico"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will it throw an error if no favicon is present?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A favicon has been included in the repo

Copy link
Collaborator

@sumitramanga sumitramanga left a comment

Choose a reason for hiding this comment

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

Just one thing to consider. Thank you for implementing my feedback 😊

Copy link
Collaborator

@sumitramanga sumitramanga left a comment

Choose a reason for hiding this comment

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

Thank you for the quick turn around! :shipit:

@miquelbeltran miquelbeltran merged commit a80e8dd into develop May 20, 2024
5 checks passed
@miquelbeltran miquelbeltran deleted the 20240517-cleanup branch May 20, 2024 04:38
@miquelbeltran miquelbeltran mentioned this pull request May 23, 2024
10 tasks
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.

3 participants