Skip to content

Conversation

mcasimir
Copy link
Collaborator

Jan-31-2022.13-03-52.mp4


describe('with SearchParams', function () {
beforeEach(function () {
cleanup();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why in the beforeEach and not afterEach?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

looks like a leftover

}
>
<Cell>
<Cell className={optionNameCellStyles}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I asked this last time, but didn't get an answer: why are we using this table here at all? Seems like we are complicating layout both visually and from the code perspective, and the only thing this adds is the table headers that just duplicate the placeholders in the input fields

Copy link
Collaborator Author

@mcasimir mcasimir Jan 31, 2022

Choose a reason for hiding this comment

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

Yeah is kinda tricky to align stuff, what you say makes sense to me. I don't know if this has to be a table or we could remove it and keep only the inputs. @Sgrinfy this is more a question for you.

I would keep it as is for now in this PR, and possibly change it in a follow-up one, there are a few other fixes to the new form that would be great to get in before.

@mcasimir mcasimir merged commit 53b8048 into main Feb 1, 2022
@mcasimir mcasimir deleted the remove-advanced-option-warning branch February 1, 2022 09:09
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.

3 participants