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

Added dashboard for Ben Murrell #1180

Merged
merged 2 commits into from
Aug 28, 2024
Merged

Added dashboard for Ben Murrell #1180

merged 2 commits into from
Aug 28, 2024

Conversation

misterabdullahAziz
Copy link
Contributor

@misterabdullahAziz misterabdullahAziz commented Aug 26, 2024

@Ziip-dev
Copy link
Member

Please provide at least a short description to let me know what is the context of this PR.
As I haven't worked on this, I don't know any details (e.g. clickable link to Jira issue, purpose of the javascript shortcode, etc.)

@Ziip-dev
Copy link
Member

Also, maybe I missed that during the meetings, any specific reason to put the javascript snippet in a shortcode file?

@misterabdullahAziz
Copy link
Contributor Author

Please provide at least a short description to let me know what is the context of this PR.

As I haven't worked on this, I don't know any details (e.g. clickable link to Jira issue, purpose of the javascript shortcode, etc.)

Link for issue, I have added in the description.
There was need to add new dashboard page given by Ben Murrell group (attached in jira issue).

The purpose of the javascript was to get last updated date for the plots. To be consistent with other dashboards I used shortcode as per suggested by senthil.

@misterabdullahAziz
Copy link
Contributor Author

Also, maybe I missed that during the meetings, any specific reason to put the javascript snippet in a shortcode file?

To be consistent with other dashboards.

- Switched from XMLHttpRequest to fetch API (more modern replacement).

- Used async syntax (eliminates the need for callbacks and .then()
  chains, marginal here but still good practice).

- Removed `onreadystatechange`: with Fetch API we can simply wait for
  the fetch promise to resolve.

- Simplified error handling by using a `try/catch` block.

- Removed redundant `var` declarations for `const` and `let` variable
  declarations (modern javascript).

- Extracted a helper function `updateModifiedDateElement` to reduce code
  duplication.

- Early `return` in the `if (commits.length === 0)` block to avoid
  executing the rest of the function.

- Removed unnecessary `else` clause since we're returning early in the
  `if (commits.length === 0)` block.
Copy link
Member

@Ziip-dev Ziip-dev left a comment

Choose a reason for hiding this comment

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

I've refactored the javascript shortcode for higher readability, reusability, and maintainability. If you want to have a final look (I left explaining comments in the commit description).

On my side everything is okay :)

@Ziip-dev Ziip-dev merged commit 1270441 into develop Aug 28, 2024
3 checks passed
@Ziip-dev Ziip-dev deleted the FREYA-888/Abdullah branch August 28, 2024 12:01
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