Updating Meta Tags

[AnnMarieW]

Description

Closes #176

Checklist

• I have not referenced individuals, products or companies in any commits, directly or indirectly
• I have not added data or restricted code in any commits, directly or indirectly
• I have updated the docstring of any public function/class/model changed
• I have added tests to cover my changes (if applicable)

Types of changes

• Docs/refactoring (non-breaking change which improves codebase)
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Notice

• I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

  • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
  • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
  • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

[AnnMarieW]

This first commit is just a demo of how a logo and description can be included in the meta tags.

[AnnMarieW]

I'd like to do the docs in a separate PR, so this is ready to review.
I allowed for the option to set a path to an image because the id can have spaces which is not ideal for filenames.
Please let me know what sort of tests you would like.

[antonymilne]

This looks great to me, thank you very much for your first PR @AnnMarieW.

Probably we should remove the get_relative_path(f"/{STATIC_URL_PREFIX}/images/app.svg") for now since the point you raised against it is a very valid concern!

As for tests, here's where our current tests for Dashboard.pre_build are:

vizro/vizro-core/tests/unit/vizro/models/test_dashboard.py

Line 149 in 8d82ebf

class TestDashboardPreBuild:

. I have some ideas for tests that we could write but am very curious to hear what tests you would propose? Our current testing setup is quite new and could definitely be made more rigorous in places - something I'd like to us to improve in the new year.

I was looking through the Dash tests that you linked me to. Am I right in thinking that all the tests on Dash pages are contained here? And there's no explicit tests for _infer_pages itself? Instead it's this test and the following one that test the behaviour of (a) no image is supplied and app.jpeg is used and (b) birds.jpeg is used.

[AnnMarieW]

Yes most of the Pages tests are in the links you provided. The Dash tests confirm the meta tags are created as expected based on the dash.page_registry. For the Vizo tests, it's probably enough to test that the dash.page_registry is correct. I'll take a look at adding some tests in the Dashboard.pre_build section you pointed to.

[antonymilne]

@AnnMarieW Here's the sort of thing I'd suggest for tests (haven't tried it out at all yet, no doubt has typos and errors, but hopefully you get the idea). Take a look at #228 first for context 🙂

    def test_page_registry_with_description(self, vizro_app, mocker):
        mock_register_page = mocker.patch("dash.register_page", autospec=True)
        vm.Dashboard(
            pages=[vm.Page(title="Page 1", components=[vm.Button()], description="My description")]
        ).pre_build()

        mock_register_page.assert_any_call(
            module=page_1.id,
            name="Page 1",
            title="My dashboard: Page 1",
            path="/",
            order=0,
            description="My decription",
            layout=mocker.ANY,  # partial call is tricky to mock out so we ignore it.
        )

    @pytest.mark.parametrize(
        "image_filename", ["app.png", "app.svg", "folder/app.png", "folder/app.svg", "logo.png", "logo.svg"]
    )
    def test_page_registry_with_image(self, page_1, mocker, tmp_path, image_filename):
        vizro_app = Vizro(user_assets_folder=tmp_path)
        (tmp_path / image_filename).touch()
        mock_register_page = mocker.patch("dash.register_page", autospec=True)
        vm.Dashboard(pages=[page_1], title="My dashboard").pre_build()

        mock_register_page.assert_any_call(
            module=page_1.id,
            name="Page 1",
            title="My dashboard: Page 1",
            path="/",
            order=0,
            image=str(tmp_path / image_filename),
            layout=mocker.ANY,  # partial call is tricky to mock out so we ignore it.
        )

Plus one which tests what happens if you have both app.png and logo.png in the assets folder.

[AnnMarieW]

@antonymilne - I got all your suggested tests added 🎊

[antonymilne]

Amazing work, thank you very much for your contribution @AnnMarieW! 🙏

I've made lots of very small suggested changes, but it's nothing big.

You're going to have to do a couple of extra bits in order to pass our CI checks but I won't say explicitly what that as an experiment to see whether it's easy to figure out how to fix it from our current error messages (FYI @maxschulz-COL). But please do just say if it's not clear what to do or you need any help getting CI to pass 🙂

[AnnMarieW]

Hi @antonymilne
I see the following error message in the failing test. I'm running Python 3.10 and when I run the test locally everything passes. Do I need to install and run Python 3.11 locally too?

[antonymilne]

@AnnMarieW hopefully you don't need to install Python 3.11 to get this to pass. For now ignore the error message and just running hatch run schema locally and then pushing to see if it fixes the check.

[huong-li-nguyen]

This looks great! Thank you so much you two @AnnMarieW and @antonymilne 👍

@AnnMarieW - don't forget to add yourself to the list of contributors here. We would love to add you to our list given all your valuable feedback! 🚀

[maxschulz-COL]

Also had a quick read through - looks really good. Small code change that will make a huge difference in many places 💪

As mentioned by Antony, do let us know how you get on from here, very curious if the remaining PR tasks are self-explanatory.

Topic aside: I have started work on the AG Grid for Vizro - would be great to have a catch-up about that in the next year as I recall you helped a lot on that feature on the Dash side right?

[AnnMarieW]

Hi @maxschulz-COL
Looks like all the checks are green now ✔️
Just need to fix the conflicts 😨

I would love to see AG Grid in Vizro, and yes, I'm very involved in that project at Plotly. I look forward to chatting with you about that whenever you have time 🙂