Add layout: sidebar and app header #2
Conversation
Toggling the SideBar's 'visible' prop to false, or initializing it as false, results in the CoreUI Sidebar component emitting a 'hide' event (a callback fired after hiding). Sometimes, we want to obey this 'hide' emit: specifically, when the user, on mobile, taps another part of the page. But if the 'hide' emit is received as a result of the user directly requesting the sidebar to be hidden, we should NOT then try to toggle the sidebar a second time, which would undo the user's action.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
I don't know why, but Playwright on CI couldn't see the test server, until I specified the host and port like this. It's as if the name 'localhost' isn't mapped to '127.0.0.1'.
Playwright CI couldn't find my element on CI that it could find locally, because, for a reason I haven't established, the data-testid attribute only comes through locally.
and remove unused tmate as it wasn't working
david-mears-2
changed the title
The header had spilled over onto two rows because of not enough horizontal space
This will also handle formatting as well as linting, via eslint stylistic
This reverts commit 6949efb.
|
I reverted out the 'do linting' commit because it completely clutters up the PR diff. I can add it back after reviews, and then the 'lint' workflow will pass, as shown by the green tick on that commit. In the meantime, to make it readable, you'll want to switch off annotations by pressing the shortcut |
There was a problem hiding this comment.
Looks great - really slick.
I'm not completely convinced by having the logos appear in the sidebar, that Imperial "I" is quite distracting... but I guess they have to go somewhere. We put them on the About page in arbomap - will you need one of those too? - so maybe could go there if acceptable?
The ? is going to be the Help link? It would look more consistent to make that a logo too I think, and give it a tooltip.
I think the breadcrumbs would look a little cleaner without underline for links, and without the grey horizontal line between header and breadcrumb on smaller screen. But that's very subjective...
Unit tests for components going in a separate ticket?
| }) | ||
| </script> | ||
|
|
||
| <template> |
There was a problem hiding this comment.
I've always preferred putting the template above the script, as it feels more natural to see your static markup before the dynamic operations which work upon it. But again, this is quite subjectve!
There was a problem hiding this comment.
I agree actually, so I'm now overriding the lint/format rule that caused this.
pages/comparison.vue
Outdated
| breadcrumbs: [ | ||
| { text: 'Base scenario', href: '#' }, | ||
| { text: 'Explore more scenarios', href: '#' }, | ||
| { text: `By policy response` } | ||
| ] | ||
| }) |
There was a problem hiding this comment.
Yeah, it feels like the router ought to be giving us breadcrumb information.
pages/index.vue
Outdated
|
|
||
| <script setup lang="ts"> | ||
| definePageMeta({ | ||
| hideBreadcrumbs: true |
There was a problem hiding this comment.
..again, it feels like the breadcrumb component itself might make decision about hiding breadbrumb if only one level of route etc, rather than page defining it in the meta.
I'm not sure if we will have an 'About' page in so many words. We'll have a page that explains methodology and links to the paper, which is similar, but might not get called 'About' and I'm not sure the logo/acknowledgements should go on that page. I've been led to understand that branding it with the name Jameel is important. For now, I will remove the Imperial I so the logo only shows when the sidebar is open (I wasn't convinced about my solution for the distractingness of the I - I was reducing its opacity). |
Agreed. I didn't because there wasn't such an icon in the library I was using. Tooltip (you mean just the word 'Help' or something?) comes in #3. I'm going to add an svg from font awesome to assets. |
|
I think I've addressed all comments. I merged into this branch the new way for sidebars to work. I intend to work out how we 'implement breadcrumbs properly' in this ticket. After final reviews, I'll run |
CoreUI components weren't being found by the app in tests, as they weren't available in the Nuxt context for some reason. Telling Nuxt to auto-import these components, rather than loading them in a plugin, made them available in tests.
There was a problem hiding this comment.
Looks good! Think I preferred the breadcrumb left/center rather than on the right.
When you enable the formatter/linter, can you make it consistent with our other web apps?
i.e. double quotes, semicolon line endings. We're not completely consistent on indents - I prefer 4 spaces, but we also have 2 spaces...
layouts/default.vue
Outdated
| // If this is the 'hide' emitted on page load, we un-do it. | ||
| // This is because the CoreUI Sidebar component emits a 'hide' event on page load, which we | ||
| // don't want to obey for larger screen sizes. |
There was a problem hiding this comment.
If we always just want to ignore that initial hide, it feels like it might be nicer to handle that in the Sidebar component, rather than making the parent layout deal with it. Could be similar logic - check if this is our first hide event, and if it is, then don't emit it.
There was a problem hiding this comment.
Good point. I've now refactored this, using defineModel()
tests/components/SideBar.spec.ts
Outdated
| // Reviewers - I wonder if these tests are too brittle | ||
| // (tied closely to the component's implementation / would change too often) |
There was a problem hiding this comment.
There's a couple of schools of thought, and a couple of styles of component tests which we have. This sort is more clearly a unit test, where you mock out child components (shallow mount) and identify them using implementation details, like component type, class or id.
There's also the testing-library approach, which are more like integration tests, where you do a deep mount, and test the behaviour of the whole component, using things like role or visible text to identify parts to engage with - so these are more clearly behavioural tests, and map more closely to user experience.
Personally I find the latter type much more frustrating to write, and I don't think the standard way to deal with that frustration (using data-testid) really gets you anything much less brittle. Testing by text is going to be pretty brittle too.. But others are of differening opinions - I think Anmol tends to advocate for the testing-library approach...
In practice, the component structure doesn't change that often, and when it does the behaviour/tests are likely to nee dto change then anyway...
There was a problem hiding this comment.
Hmm, well my thoughts are that I already have e2e tests on the overall behavior of the sidebar, making my below tests of the implementation somewhat redundant -- except in that unit tests give quicker feedback about regressions (not only that they are quicker to run but also quicker to pinpoint what broke).
There was a problem hiding this comment.
I think unit tests are good for fine detailed test work - you might test edge cases that just involve that component in unit. e2e tests should test the main use cases work end to end, and any behaviours that require multiple components to coordinate in complex ways.
|
Code looking good to me now - did you want to fix up the lint build before merging? |
See also the earlier commit 'Update linter/formatter for bugfix patch'
NB: See this comment for how to make the PR diff readable
PR description
This PR adds what is common between all pages: an openable/closable sidebar, and an app header. These should work on all kinds of devices.
You can even test it directly on your mobile device using your local network by following the instructions in the README:
It also introduces github action workflows for testing and linting.
There aren't any component tests for the BreadCrumb component because I'm not yet sure how that will be implemented - treat it as an aspect of styling for now. (I created a separate ticket for that.)
Pages that exist
The blank 'pages' you'll be able to visit are determined by what's in the
pages/directory, that is: