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

Voucher dropdown + Miscellaneous fixes #133

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

julesfll
Copy link
Collaborator

@julesfll julesfll commented Oct 27, 2020

I have the frontend for the voucher done (still need to do redux/api) and also added a variety of tweaks/fixes:

  • Fixed NestedDOM <a> warning
  • Fixed NestedDOM <td> <button> warning
  • Fixed fill-rule warning
  • Readded scrollable to make modal scroll correctly
  • Cleaned up padding css in SearchTool.js
  • Converted help text to Bootstrap component
  • Fixed overlap with sidebar and table and made it more responsive
    • Sidebar before had fixed width, changed to bootstrap breakpoints
    • There is still a gutter on the right that I need to fix that causes horizontal scrolling

demo

@julesfll
Copy link
Collaborator Author

Seems like it broke some tests, not exactly sure what though

Copy link
Collaborator

@billyhunt billyhunt left a comment

Choose a reason for hiding this comment

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

Nice work! Maybe fix some of the stuff I mentioned if you feel like it.

frontend/src/components/Navbar.js Outdated Show resolved Hide resolved
}
onSetPage={(index) => props.pagination.page = index}
onSetPage={(index) => (props.pagination.page = index)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure what is going on here. You are setting the prop to index? Should you be setting state?

Copy link
Collaborator Author

@julesfll julesfll Nov 6, 2020

Choose a reason for hiding this comment

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

I'm not exactly sure- maybe it's intended as a Redux update? Was someone working on fixing the pagination? Otherwise I can look into this

<Col xs="10" style={{ paddingLeft: "15px" }}>
Pets allowed
</Col>
<Col xs="10">Pets allowed</Col>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did it not need the padding anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved the padding "up a level"- the class name px-3 adds the padding to the parent instead of each child having their own padding

@@ -11,11 +11,11 @@ const bootstrapSearch = () => {
xmlns="http://www.w3.org/2000/svg"
>
<path
fill-rule="evenodd"
fillRule="evenodd"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this doing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure, I think it has to do with SVG formats; just changed it because it was throwing a warning (because React prefers camel-cased params)

frontend/src/style/SearchTool.css Outdated Show resolved Hide resolved
@billyhunt
Copy link
Collaborator

Looks like you have to update your snapshots

@julesfll
Copy link
Collaborator Author

julesfll commented Nov 7, 2020

Looks like you have to update your snapshots

Oh ok! How would I do that?

@billyhunt
Copy link
Collaborator

Sorry I missed this. Did you get it figured out? Slack me tomorrow if not.

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.

2 participants