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

Clarify storybook bug #1186

Merged
merged 1 commit into from Jun 1, 2021
Merged

Conversation

luke-hill
Copy link
Contributor

Ronseal PR

Copy link
Contributor

@davidrapson davidrapson left a comment

Choose a reason for hiding this comment

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

👍

(Small aside, I'm always unsure what you mean by "Ronseal PR", if possible adding a real description even if the change feels obvious is super helpful. Thanks 🙏)

@@ -1,3 +1,4 @@
# NB: Due to a Storybook issue, these tests won't run on Old Safari / iOS
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 actual issue? Longer term we could probably start running these tests against the demo/ app which is a much simpler and more realistic environment than storybook but we should chat about that first 🤓

Copy link
Contributor Author

@luke-hill luke-hill Jun 1, 2021

Choose a reason for hiding this comment

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

Go for it. Happy to let you guys decide on this. You probably know better.

P.S. Showing my age but: https://www.youtube.com/watch?v=OkGaq9xiQZY

EDIT: As for the error proper. I'm not sure. But @davidsauntson seemed to know about it. Probably something javascripty and legacy appley?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hah, I see. Didn't make that connection.

Yeah, that probably makes sense it'd be some compatibility thing. Storybook doesn't have a very good cross-browser story. No need to switch now, but as we've migrate more components into the Rails engine there will be a pretty decent test environment within the demo app. Something to come back to.

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've popped a new Phase I ticket in the backlog for this. Happy to tackle this whenever you feel is appropriate. I don't envisage it being a lot of work our end to migrate the tests (Probably just altering some page urls / ruby setup code).

@luke-hill luke-hill merged commit 25f6f8c into master Jun 1, 2021
@luke-hill luke-hill deleted the bugfix/np-2123-clarify-storybook-bug branch June 1, 2021 15:23
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.

None yet

2 participants