ETT-1154: don't submit search if there are errors #138
Conversation
carylwyatt
left a comment
carylwyatt
left a comment
There was a problem hiding this comment.
This looks good to me! Thanks :)
| } | ||
| console.log(url.toString()); | ||
| if (window.xyzzy) { |
There was a problem hiding this comment.
@aelkiss Do you happen to know what this xyzzy thing is about? I feel like I've seen this somewhere else before, but I'm not sure what it is. You removed it here, so I assume you know what it does (or doesn't do in this case)?
My guess is that it was some kind of debugging thing Roger used locally to stop the search from executing during development, but I don't really know.
There was a problem hiding this comment.
I just confirmed my own suspicions by setting window.xyzzy = true in the browser. I bet that's what it was.
There was a problem hiding this comment.
I couldn't find any other references to xyzzy, so I went ahead and removed it. I had the same guess about what it was for.
|
I'm going to rebase this on ETT-827-advanced-search, then look at adding a storybook test for this case, then ask for re-review. I'll set to draft until #136 is merged. |
aelkiss
force-pushed
the
ETT-1154-adv-search-validation
branch
from
2c327d7 to
82bc9b2
Compare
aelkiss
force-pushed
the
ETT-1154-adv-search-validation
branch
from
82bc9b2 to
f4e7d17
Compare
|
This is now based on ETT-827-advanced-search and has storybook tests; ready for re-review. I figure after #136 is merged we can rebase against main. |
|
@carylwyatt Ready for re-review |
I think this does need a rebase against main. The storybook is catching errors for things I already fixed in those a11y bugs I merged last week, and it's claiming the tests I wrote for Advanced Search are unreviewed... the whole thing is weird. I manually re-ran the chromatic build just in case there was a weird mis-match, but I think it just needs to be rebased. |
* Ensure form isn't submitted * Add storybook stories for invalid date & missing search
aelkiss
force-pushed
the
ETT-1154-adv-search-validation
branch
from
f4e7d17 to
2d96e3b
Compare
|
Rebased & force pushed. |
carylwyatt
left a comment
carylwyatt
left a comment
There was a problem hiding this comment.
Thanks for the rebase, that fixed the chromatic issues. This looks good!
@carylwyatt I think we could add a test for this along the same lines as the stuff that was done on #136 but I think we'll want that branch merged before trying to add the test. I'll leave this as draft until #136 is done but let me know what you think.