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

About Page #56

Merged
merged 23 commits into from
Oct 25, 2024
Merged

About Page #56

merged 23 commits into from
Oct 25, 2024

Conversation

absternator
Copy link
Contributor

@absternator absternator commented Oct 16, 2024

This PR creates About page...The text is still a WIP and is waiting updates

DO NOT merge until about text is updated

image

Uploading image.png…

Copy link

codecov bot commented Oct 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.19%. Comparing base (156fded) to head (072a792).
Report is 27 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
+ Coverage   88.07%   88.19%   +0.11%     
==========================================
  Files          46       47       +1     
  Lines        2206     2227      +21     
  Branches      301      298       -3     
==========================================
+ Hits         1943     1964      +21     
  Misses        251      251              
  Partials       12       12              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@david-mears-2 david-mears-2 left a comment

Choose a reason for hiding this comment

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

Thanks, this is great. I left a lot of comments but most are suggested text changes that I hadn't seen before I went through with a fine-tooth comb.

pages/about.vue Show resolved Hide resolved
pages/about.vue Outdated
</p>
<h4>Contact us:</h4>
<p>
If you have any queries regarding DAEDALUS Explore, please contact: TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Leaving a comment to make sure we resolve this before merging in

Comment on lines 1 to 11
import About from "@/pages/about.vue";
import { mount } from "@vue/test-utils";
import { describe, expect, it } from "vitest";

describe("about page", () => {
it("should render about page", () => {
const component = mount(About);

expect(component.text()).toContain("About");
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is the equivalent in the style of Nuxt test utils. It passes.

Suggested change
import About from "@/pages/about.vue";
import { mount } from "@vue/test-utils";
import { describe, expect, it } from "vitest";
describe("about page", () => {
it("should render about page", () => {
const component = mount(About);
expect(component.text()).toContain("About");
});
});
import About from "@/pages/about.vue";
import { mountSuspended } from "@nuxt/test-utils/runtime";
import { describe, expect, it } from "vitest";
describe("about page", () => {
it("should render about page", async () => {
const component = await mountSuspended(About);
expect(component.text()).toContain("About");
});
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course if we do automate some of the about page text then that deserves a unit test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry wondering what the mountSuspended does? because it passes with regular mount?

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn’t make an immediate difference in this case. It’s the standard way of mounting a component with Nuxt test utils. It wraps ‘mount’ from Vue test utils while “allowing access to injections from your Nuxt plugins“.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh okay are we using plugins?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Pinia is a plugin.

Copy link
Contributor

Choose a reason for hiding this comment

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

@absternator Could you add to this unit test for the {{ numberOfPandemics }} and {{ numberOfCountries }} calculations?

pages/about.vue Outdated Show resolved Hide resolved
pages/about.vue Outdated
Comment on lines 14 to 16
<li>Prevalence</li>
<li>Hospital demand</li>
<li>Deaths</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, these three (but not 'economic losses' as that is separate from the time series) can be taken straight out of metadata: see mocks/responses/metadata.json under time_series to clue you into how to get that out of the appStore. As a result of doing that change, you'll already see that we've added a new time series that hadn't yet made it into the about page's text!

Copy link
Contributor

Choose a reason for hiding this comment

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

Since updates to time series, which you can see in the new version of the metadata.json file now on main, these now need to be drawn from appStore.metadata?.results.time_series_groups instead of appStore.metadata?.results.time_series

pages/about.vue Outdated Show resolved Hide resolved
pages/about.vue Outdated Show resolved Hide resolved
pages/about.vue Outdated

<p>
<strong>pictogrammersMaterialBacteria.svg</strong><br>
Icons by
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Icons by
Icon by

Copy link
Contributor

Choose a reason for hiding this comment

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

This was missed out

assets/img/prepare-logo.png Outdated Show resolved Hide resolved
pages/about.vue Outdated Show resolved Hide resolved
@david-mears-2
Copy link
Contributor

david-mears-2 commented Oct 17, 2024

Btw playwright visual testing is very brittle at the moment on all branches, partly because the model itself is constantly being updated so the data being displayed is changing!

This commit refactors the about page by adding the Jameel community logo and fixing optional chaining in the script setup section. The Jameel community logo is now displayed on the page, enhancing the visual appeal. Additionally, the optional chaining for retrieving the number of pandemics and countries from the app store metadata has been fixed to prevent potential errors.
pages/about.vue Outdated Show resolved Hide resolved
@david-mears-2
Copy link
Contributor

When this is merged with the code for the globe, that will by default still be visible on this page. I suggest doing something like I did here for the sake of a demo.

@david-mears-2
Copy link
Contributor

On small screens the logos stack vertically in a suboptimal way - needs a little more space, and needs centering

image

@EmmaLRussell
Copy link
Contributor

Just to keep track of everything in YT, I've added a ticket for this: https://mrc-ide.myjetbrains.com/youtrack/issue/JIDEA-172/About-page

Copy link
Contributor

@EmmaLRussell EmmaLRussell left a comment

Choose a reason for hiding this comment

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

Since this and the globe ticket are likely to be merged separately, I've made a separate ticket for hiding globe on this page: https://mrc-ide.myjetbrains.com/youtrack/agiles/103-73/current

Copy link
Contributor

@EmmaLRussell EmmaLRussell left a comment

Choose a reason for hiding this comment

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

Looks good!

I think this comment from David still needs to be done (it's a change requested on a change already made)

Comment on lines 51 to 52
const vueSelects = component.findAllComponents(VueSelect).slice(0, 2);
expect(vueSelects.length).toBe(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the idea with this change? Are there more vue selects than there were? It seems odd to slice an array, and then check its length that you've just modified!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i had initially done something else but reverted this now

Copy link
Contributor

@EmmaLRussell EmmaLRussell left a comment

Choose a reason for hiding this comment

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

Still don't think that R&D line is quite there, but go ahead and merge when that is updated.

pages/about.vue Outdated Show resolved Hide resolved
Co-authored-by: Emma Russell <[email protected]>
@absternator absternator merged commit de34cd7 into main Oct 25, 2024
6 checks passed
@absternator absternator deleted the about-page branch October 25, 2024 13:09
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