-
Notifications
You must be signed in to change notification settings - Fork 269
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
[Modal,NftCard,Select,Input,]: Bug fix and feature added #983
Conversation
…t/add-modal-customize
🦋 Changeset detectedLatest commit: 4958554 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
@@ -167,7 +167,7 @@ const DivStyledOverlay = styled.div` | |||
bottom: 0; | |||
display: none; | |||
left: 0; | |||
position: absolute; | |||
position: fixed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this correct? wont the overlay be fixed to the page instead of the parent? does this casue an issue when you scroll the page?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the overlay is used to close the select menu if someone clicks outside of the select.
If absolute is used it takes the parent size which is usually not the screen size so the menu won't be closed. (checkout select component in admin playground it won't close even if clicked outside)
But if fixed is used it take up entire screen and if user clicks anywhere the select component will be closed
@@ -12,6 +12,7 @@ const VisibilityToggle: React.FC<IVisibilityToggleProps> = ({ | |||
className="input_visibility" | |||
data-testid="test-input-icon-visibility" | |||
onClick={() => onClick()} | |||
tabIndex={-1} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why minus -1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on password type input, when you click tab it focuses on the password icon to avoid this negative index is being used. (Never seen any site implement focusing on the password icon)
https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/tabindex
A negative value (usually tabindex="-1") means that the element is not reachable via sequential keyboard navigation, but could be focused with JavaScript or visually by clicking with the mouse. It's mostly useful to create accessible widgets with JavaScript.
@@ -68,7 +68,7 @@ const SelectMenuList: React.FunctionComponent<ISelectExtendedProps> = ({ | |||
if (wrapper) { | |||
const el = wrapper.querySelector(selector); | |||
if (el) { | |||
el.scrollIntoView(false); | |||
el?.scrollIntoView?.(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i didnt know scrollIntoView
was a function now, we used to do this manually. It seems well supported but there is some notes about Safari
https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the function is supported only the options parameter is not, just to be on the safer side I added optional chaining too.
I tested it on safari and it is working 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some small questions more than comments. Also it would be nicer to break something like this into smaller MRs if possible 👍
yeah sorry, my bad I thought I was working on different branches but only realized that was not the case after everything was done. Will take care in future 🙌🏻 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢 Ship it!
name: 'Pull request'
about: A new pull request
New Pull Request
Checklist
Issue Description and Solution Description
Solution:
metadata.file
in api result, added fallback image if there is any issue in loading ipfs image (images don't load on palm network sometimes)Solution: Added more conditions in image display logic
fixed
to overlay elementIssue:
tabIndex={-1}
to password button to avoid it on tab click.Issue: when used with react hook form, the error message is updated after first render but its not updated in the input component