Skip to content
This repository has been archived by the owner on Jun 21, 2022. It is now read-only.

Bootstrap update to v5 of recipe, dish and purchase list editor #183

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

kuro610
Copy link
Collaborator

@kuro610 kuro610 commented Jun 1, 2022

Closes #130
Closes #131
Closes #133

@kuro610 kuro610 requested a review from dboehmer June 1, 2022 19:55
@kuro610 kuro610 mentioned this pull request Jun 1, 2022
@kuro610
Copy link
Collaborator Author

kuro610 commented Jun 1, 2022

@dboehmer The update of the purchase list editor makes 1 test failing "change item total" from t/controller_Item.t. As far as I understand the test it should work, but it could be that the test references a wrong form in line 52. So could you please check this.-

@dboehmer
Copy link
Owner

dboehmer commented Jun 2, 2022

The project page shows an empty alert box for this test project when not logged in:

image

@dboehmer
Copy link
Owner

dboehmer commented Jun 2, 2022

@dboehmer The update of the purchase list editor makes 1 test failing "change item total" from t/controller_Item.t. As far as I understand the test it should work, but it could be that the test references a wrong form in line 52. So could you please check this.-

I tried to change arguments to submit_form_ok to the form with ID total-1 but it still doesn’t work. Maybe WWW::Mechanize doesn’t like the empty <form>s with form elements outside an attribute form="total-1"? I am not used to that syntax, too.

@kuro610
Copy link
Collaborator Author

kuro610 commented Jun 2, 2022

Before the update the form only had a name ("total") (which would also not be unique when we have multiple items on our purchase list) and no id. I added the id so that I can use the form elements outside with the form attribute. The unit conversion form works also with this approach (form elements outside the form), but maybe this form is not covered by any test.

@kuro610
Copy link
Collaborator Author

kuro610 commented Jun 2, 2022

If I test the total form by hand it works.

@kuro610
Copy link
Collaborator Author

kuro610 commented Jun 2, 2022

Here are some information about the form attribute on form elements.

@kuro610
Copy link
Collaborator Author

kuro610 commented Jun 2, 2022

The project page shows an empty alert box for this test project when not logged in:

image

I removed the empty alert box.

background-color: #4CAF50;
}
background-color: var(--cc-green);
}
Copy link
Owner

Choose a reason for hiding this comment

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

missing trailing newline

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added the missing newline

@kuro610
Copy link
Collaborator Author

kuro610 commented Jun 5, 2022

I fixed the test. It looks like that Mechanize didn't support the form attribute on inputs.

I think now is the branch ready to be merged

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants