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

10516 - cleanup the dtui in-page nav component #184

Merged
merged 7 commits into from
Feb 13, 2024
Merged

Conversation

anjenkin
Copy link
Contributor

Check List Before Merging

Author

  • Story Book has been rebuilt (new build artifacts in /docs)
  • Components have been rebuilt (new build artifacts in /dist)

👆These can be done by executing npm run build and committing the build artifacts.

  • All new components have been exported in /index.js (if applicable)
  • Design Review Complete
  • Relevant Draft release notes have been updated describing these changes (use this template to document your changes) (if applicable)
  • Integration Status Updated on relevant documentation page (if applicable)
  • index.d.ts updated with new prop types defined for new components. (if applicable)

Reviewer

  • Code review

@brianpetway brianpetway self-assigned this Feb 13, 2024
* inPageNavHelper.js
* Created by Andrea Blackwell 01/16/24
*/
import {useCallback} from "react";
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this isn't used

export const checkIsOverflow = (ulEl, padding) => {
let left = false;
let right = false;
console.log(ulEl, padding);
Copy link
Contributor

Choose a reason for hiding this comment

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

a few console logs in this block

statements: -10
}
},
// coverageThreshold: {
Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean to leave this commented out?

@brianpetway
Copy link
Contributor

A few inline comments.

The testing strategy is very clever, that must have taken a minute to arrive at.
I know you said you didn't get to add as many tests as you had hoped; in the future we should add tests for different screen sizes, with the different numbers for padding, since we've had issues around that

@brianpetway
Copy link
Contributor

Also, do you need to add the new dtui version number in this branch?

@anjenkin
Copy link
Contributor Author

Yeah I could see the tests I added being helpful during development given the issues with calculating the overflow but not sure how helpful this is now. we could add test for the padding issue in another ticket.

@brianpetway brianpetway merged commit aa8fab2 into master Feb 13, 2024
@brianpetway brianpetway deleted the mod/10516-cleanup branch February 13, 2024 19:33
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