Merged
Conversation
Closed
|
floehopper
force-pushed
the
add-support-for-block-to-text-app
branch
from
7a0bf73 to
b91556c
Compare
floehopper
temporarily deployed
to
previews/add-support-for-block-to-text-app
GitHub Actions
Inactive
|
floehopper
force-pushed
the
add-support-for-block-to-text-app
branch
from
b91556c to
ebe35eb
Compare
floehopper
temporarily deployed
to
previews/add-support-for-block-to-text-app
GitHub Actions
Inactive
|
floehopper
force-pushed
the
add-support-for-block-to-text-app
branch
from
ebe35eb to
b967f42
Compare
floehopper
temporarily deployed
to
previews/add-support-for-block-to-text-app
GitHub Actions
Inactive
floehopper
force-pushed
the
add-support-for-block-to-text-app
branch
from
b967f42 to
0ab0ae9
Compare
floehopper
temporarily deployed
to
previews/add-support-for-block-to-text-app
GitHub Actions
Inactive
|
floehopper
force-pushed
the
add-support-for-block-to-text-app
branch
from
0ab0ae9 to
db6ebd6
Compare
floehopper
temporarily deployed
to
previews/add-support-for-block-to-text-app
GitHub Actions
Inactive
|
floehopper
force-pushed
the
add-support-for-block-to-text-app
branch
from
db6ebd6 to
4662889
Compare
floehopper
temporarily deployed
to
previews/add-support-for-block-to-text-app
GitHub Actions
Inactive
|
I noticed that this test was commented out and since I'm about to make changes in this area of the codebase, I thought it would be worthwhile fixing it.
Based on changes in this commit [1]. This changes `WebComponentProject` so it displays only the `Output` component instead of the `Project` or `MobileProject` component. This closely matches the existing behaviour of the `EmbeddedViewer` component. And in fact we also style the `Output` component like the latter by wrapping it in the "embedded-viewer" CSS class and importing the `EmbeddedViewer.scss` styles. The immediate motivation for this change is to support the Block-to-Text app [2], but we also hope to replace the `EmbeddedViewer` route with the web component in some kind of embedded mode. The exercise in the Block-to-Text app uses this attribute [3] to display the output generated from running the code. In order to use the `Output` component in this new web component context, we've added `embedded` & `browserPreview` properties to it. I think this is because we need to be able to set them explicitly in the context of the web component since we can't (or don't want to) rely on setting them from the query string as compared with using the `EmbeddedViewer` in the context of an `iframe`. However, it's slightly odd to me that we've done this for `browserPreview` at this point, because we explicitly set it to `false` in `WebComponentProject` which is the default. But perhaps this is thinking ahead a bit to wanting to make these properties available as web component attributes so that we can do away with using `EmbeddedViewer` in an `iframe`. I've also added some unit tests - I've added `data-testid` attributes to a few elements to achieve this. I plan to add an e2e spec in a subsequent commit which will test all the new attributes on the web component. [1]: 28de5b8 [2]: https://github.com/RaspberryPiFoundation/block-to-text-alpha/ [3]: https://github.com/RaspberryPiFoundation/block-to-text-alpha/blob/d16a8ecc1d3da202363fb16db31f4bf93f46aa04/src/components/Editor.js#L18
Based on a curated set of changes from #901. The new `assets_identifier` attribute is intended to be used in conjunction with the `code` attribute so that the supplied source code can reference images included in the project. Setting the attribute results in a call to the `Api::Projects::ImagesController#show` action [1] in `editor-api` which renders the `app/views/api/projects/images.json.jbuilder` template [2] which makes just the images for the project available; not the source files. The motivation for this change is for the Block-to-Text app [3] which uses the new attribute in its exercise to load the images for the "bridge-prototype" project [4,5]. I've also added unit tests to test the new behaviour. I plan to add an e2e spec in a subsequent commit which will test all the new attributes on the web component. [1]: https://github.com/RaspberryPiFoundation/editor-api/blob/96f435fbd9c2b1fb35821dd98e30a97056c6501a/app/controllers/api/projects/images_controller.rb#L8-L12 [2]: https://github.com/RaspberryPiFoundation/editor-api/blob/96f435fbd9c2b1fb35821dd98e30a97056c6501a/app/views/api/projects/images.json.jbuilder [3]: https://github.com/RaspberryPiFoundation/block-to-text-alpha/ [4]: https://github.com/RaspberryPiFoundation/block-to-text-alpha/blob/d16a8ecc1d3da202363fb16db31f4bf93f46aa04/src/components/Editor.js#L15 [5]: https://github.com/raspberrypilearning/bridge-prototype
So that it overrides the content of the main component in the loaded project. This means that for example, you can set the `assets_identifier` to bring in some images from a real project, but override the main component source code in `main.py` or `index.html`. The motivation for this change is to support the Block-to-Text app [1]. Its exercise uses the `code` attribute to load the relevant code for each step and modify it as the user makes changes [2]. This is based on a curated set of changes from #901. I've also added a bunch of unit tests which hopefully explain a bit about the detail of the behaviour. In doing that I noticed what I think is a small bug when the real project has other non-default components (i.e. files other than `main.py` or `index.html`), but I plan to fix that in a subsequent commit. I plan to add an e2e spec in a subsequent commit which will test all the new attributes on the web component. [1]: https://github.com/RaspberryPiFoundation/block-to-text-alpha/ [2]: https://github.com/RaspberryPiFoundation/block-to-text-alpha/blob/d16a8ecc1d3da202363fb16db31f4bf93f46aa04/src/components/Editor.js#L17
I think this was the original intention in the changes I've included from #901, but there was a bug in the boolean logic. The `otherComponents` should include all components other than `main.py` & `index.html`. Previously it was typically not include any other components, because the `component.extension !== defaultExtension` expression was `false` for all of them, since in a Python project all the files would have the `.py` (i.e. default) extension.
Based on [1]. We're about to make a change in this area of the code. Making this change first will make it easier to see what's being changed in the subsequent commit. [1]: 4440a87
Based on [1] & #899. The motivation for this change is to support the Block-to-Text app [2] which uses the `rerunCode` method in its exercise to allow the user to run the code from the button outside the web component [3]. This mainly adds the `runCode`, `stopCode`, & `rerunCode` methods to the web component, but it also adds a check on the `webComponent` boolean state in `Output` to determine whether or not to render the `RunBar`. I don't really understand why this is needed, but having chatted to @loiswells97 we've got a nagging feeling there's a use case that does need it, so I'm going to leave it in plae for now. I've added a unit test to illustrate the new behaviour. I plan to add an e2e spec in a subsequent commit which will test the new methods on the web component. [1]: f9086a8 [2]: https://github.com/RaspberryPiFoundation/block-to-text-alpha/ [3]: https://github.com/RaspberryPiFoundation/block-to-text-alpha/blob/d16a8ecc1d3da202363fb16db31f4bf93f46aa04/src/components/Step.js#L87
Based on #915. This stores a structured version of the error in `errorDetails` in the Redux store in the `PythonRunner` `handleError` function and sends it in the payload of the "editor-runCompleted" custom event from the `WebComponentProject` component. The motivation for this change is to support the Block-to-Text app [1] which uses this to display an error message to the user in its exercise [2]. It also hides the `ErrorMessage` component in `PythonRunner` when `isSplitView` is `false`. I've added some unit tests for the new behaviour. I plan to add an e2e spec in a subsequent commit which will test all the new attributes on the web component. [1]: https://github.com/RaspberryPiFoundation/block-to-text-alpha [2]: https://github.com/RaspberryPiFoundation/block-to-text-alpha/blob/d16a8ecc1d3da202363fb16db31f4bf93f46aa04/src/components/Step.js#L53
Based on #909. The new `output_panels` attribute on the web component allows specific panels to be selected when the `output-only` web component attribute is set to `true`. Currently there are two panels: the "text" panel and the "visual" panel. You can select either or both. Note that this functionality has only currently been implemented for the `PythonRunner` and not the `PyodideRunner` or the `HtmlRunner`. The motivation for this change is to support the Block-to-Text app [1]. The exercise in the alpha version of this app displays just the "visual" panel [2]. I've added a couple of unit tests in `PythonRunner.test.js` to test the new behaviour. And I plan to add an e2e spec in a subsequent commit which will test all the new attributes on the web component. [1]: https://github.com/RaspberryPiFoundation/block-to-text-alpha/ [2]: https://github.com/RaspberryPiFoundation/block-to-text-alpha/blob/d16a8ecc1d3da202363fb16db31f4bf93f46aa04/src/components/Editor.js#L19
This adds a new e2e spec which loads the web component with its
attributes set to mimic the usage by Block-to-Text alpha app [1,2].
* The `assets_identifier` attribute is set to load the assets (but not
the code) from the "encoded-art-starter" project which includes a
`moon.png` image.
* The `host_styles` attribute is set partly because it is used by the
Block-to-Text app, but also because otherwise the output-only editor
has zero height in the Electron browser used by default by Cypress.
* The `code` attribute is set to load a simple P5 sketch which draws the
word "PASS" in green and draws the `moon.png` image on the canvas.
* The `output_only` attribute is set to `true` so that only the output
pane of the editor is displayed; not the code editor, the run bar,
etc.
* The `output_panels` attribute is set to `["visual"]` so that only the
visual output canvas is displayed; not the text output.
* A "Run code" button has been added to `src/web-component.html` to
mimic a similar button in the Block-to-Text app which has an `onclick`
handler to call the `rerunCode` method on the web component.
Note that the "embedded-viewer" CSS class that we're asserting against
is added by the `WebComponentProject` component around the `Output`
component; not by the `EmbeddedViewer` component which is currently only
used via the `/:locale/embed/viewer/:identifier` route which is used in
an iframe by e.g. the Projects app.
By asserting the results contain '{"errorDetails":{}}' we can have
_some_ confidence that the P5 sketch code has run successfully. However,
if e.g. the image is not available, the test will still pass despite an
uncaught exception being thrown. It might be worth investigating whether
Cypress can do visual regression testing to assert the canvas looks how
it is supposed to, but I'm going to say that's out-of-scope for the
moment.
[1]: https://block-to-text-alpha.pages.dev/
[2]: https://github.com/RaspberryPiFoundation/block-to-text-alpha/
floehopper
force-pushed
the
add-support-for-block-to-text-app
branch
from
4662889 to
b93d24b
Compare
|
|
This all looks good to me, in particular the addition of the e2e test in the final commit really helped me understand how everything hangs together.
I agree with this. It feels like if there were a way to reduce the number of arguments (via a combined
I wasn't really able to help with that - I think it might be worth @sra405, @loiswells97 and/or @danhalson reading through the individual commit messages (rather than the combined diff) at least to see if there's any useful context missing that we can add to the commit notes to help our future selves. But I'm going to approve this for now so that you're not blocked 👍 |
chrislo
approved these changes
Jun 3, 2024
Contributor
Author
Merged
floehopper
added a commit
that referenced
this pull request
Jun 4, 2024
### Added - Add `project_name_editable` attribute to web component (#1009) - Fires custom event when the theme changes (#1015) - Add `output_only` attribute to web component (#1019 & originally #782) - Add `assets_identifier` attribute to web component (#1019 & originally #901) - Enhance `code` attribute on web component to override project main component content (#1019 & originally #901) - Add `runCode`, `stopCode` & `rerunCode` methods to web component (#1019 & originally #899) - Send error details in "editor-runCompleted" event (#1019 & originally #915) - Return error details to web component (#1019 & originally #915) - Add `output_panels` attribute to web component (#1019 & originally #909) ### Changed - Remove unused `/embedded/projects/:identifier` route (#1013) ### Fixed - Remove unused `REACT_APP_LOGIN_ENABLED` env var (#1006) - Fix infinite remix loop when `BYPASS_AUTH` set in `editor-api` (#1007) - Fixes for docker-compose.yml (#1008) - Fix deprecation warnings in GitHub Actions (#1011) - Removed unused `isEmbedded` param from `useProject` call in `EmbeddedViewer` (#1016) - Improvements to Cypress specs in CI (#1017) - Fix warnings and verbose output when starting Webpack Dev Server (#1018) - Add e2e spec for project remix behaviour in web component (#1020) - Fix initial value of `user` in `WebComponentLoader` (#1021) - Make `authKey` in e2e web component spec more realistic (#1022) - Remove unused `ComponentStore` (#1023)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This adds a bunch of new attributes (e.g.
output_only,output_panels&assets_identifier), methods (e.g.rerunCode) and behaviour to the web component to support the Block-to-Text app.This branch is heavily based on the work in #782 which is what is what is currently being used by the alpha version of the Block-to-Text app. I've cherry-picked commits from the original work onto the latest work in
mainand curated the commits a bit to try to make them atomic and easier to follow.Detail
In each commit:
Obviously writing the explanations after the fact and not being the original author of the changes is sub-optimal, so please let me know if I've got anything wrong!
In the final commit, I've also added a new Cypress end-to-end spec which uses the existing
src/web-component.htmlpage to configure the web component in a similar way to how the Block-to-Text app uses it. The spec uses the web component to runs a simple P5 sketch, drawing some text and an image from an existing project. Hopefully this gives us some confidence that the web component will work OK with the Block-to-Text app.Notes/Questions
I haven't included the change which exposes the
embeddedproperty as a web component attribute, even though the Block-to-Text app is setting it, because I don't believe it's necessary. Please let me know if you think I'm mistaken!I do have some concerns about the number of configuration options knocking around in the code, particularly all the boolean ones where I suspect there may well be some overlap. I understand why it's like this, but I don't think it helps us that some configuration is done via properties, some by query string params, and some by web component attributes. Also I'm not entirely convinced that configuration options belong in a Redux store. However, I am hopeful that it will be easier to rationalize all this a bit in the 2nd step of this issue and it seems a bit too risky to do any of that at this stage.
I also wonder whether we could simplify/reduce the number of web component attributes, e.g. perhaps we could have
identifier&assetsOnlyinstead ofidentifier&assetsIdentifier; or perhaps we could even have a higher-levelmodeattribute which had a special "Block-to-Text" value which set the relevant lower-level properties...? However, I think we should probably just get the changes in this PR merged and tackle any of that later.I've run the Block-to-Text app locally against this branch of
editor-uiand worked my way through all the steps of the exercise successfully. However, I haven't done a great deal of manual testing of theeditor-uiapp or of the web component in other modes - I'm hoping that the existing automated tests will have highlighted any regressions. Let me know if you think I should do some manual testing of particular functionality before merging.Closes https://github.com/RaspberryPiFoundation/projects-ui/issues/2498.