-
Notifications
You must be signed in to change notification settings - Fork 20
fix: maps attachments for demo preview #521
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
Conversation
|
|
@garethbowen This is a quick fix to load attachments in the All Questions Form on ODK's website Web Forms Preview page, so anyone can see the questions working properly. I'm not so familiar with this piece of code, so I tried to keep the changes to a minimum. |
| export const xformAttachmentFixturesByPath = buildXFormAttachmentFixturesByAbsolutePath( | ||
| xformAttachmentFixtureEntries | ||
| ); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this not used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find any usage; maybe it's leftover code or they intended to use it in the future.
| assetsPath = new URL(relativePath, parentPathURL).pathname; | ||
| } | ||
|
|
||
| return [assetsPath, fixture]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better if we put the assets in the right place at build time and didn't have to mess with the globs here, that way production/demo/test would all run the same code. But if this works let's go with it for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and when building the dist for production, it should skip adding the forms and assets that are for dev only to keep it lighter. I think we need to restructure that fixture folder to make that separation clear and update the build process.
The Web Forms' Demo Preview doesn't show attachments because, during a demo build (
/dist-demo), all form attachments are relocated to/assets/. ODK's website takes the demo build (artifact) from this repository'smainbranch once daily and puts it in the/web-forms/app/folder. This fix doesn't impact the Web Form package distributed via npm, so I didn't include it in the changeset.I have verified this PR works in these browsers (latest versions):
What else has been done to verify that this works as intended?
I tested by running this in 3 ways, and they all work:
yarn workspace @getodk/web-forms dev- Form's assets are accessed from thecommonpackage.yarn workspace @getodk/web-forms dist-demo- Form's assets are accessed from/dist-demo/assets/./web-forms/app/assets/.This is from the ODK website (running locally):
Why is this the best possible solution? Were any other approaches considered?
It's not optimal, but it's okay for now because otherwise it would require a bigger change in how we build and run this project, which isn't the current focus (we're focusing on geo map work).
The Demo Preview and Web Forms as component/"plugin" are in the same Vue project; it's probably better to only build and start Demo Preview from
yarn workspace @getodk/web-forms dist-demo. This way, when we runyarn workspace @getodk/web-forms dev, it won't start the Demo Preview. That would ensure the form's assets are always accessible from the/dist-demo/assets/folder.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
N/A
Do we need any specific form for testing your changes? If so, please attach one.
N/A
What's changed
assets, then use that instead of thecommonpackage path.