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

design requests #462

Merged
merged 1 commit into from
Mar 5, 2024
Merged

design requests #462

merged 1 commit into from
Mar 5, 2024

Conversation

buddy-web3
Copy link
Member

No description provided.

@buddy-web3 buddy-web3 self-assigned this Mar 1, 2024
@@ -67,6 +67,7 @@
align-content: center;
justify-content: flex-start;
align-items: flex-start;
width: 100%;
Copy link
Collaborator

Choose a reason for hiding this comment

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

apply the full-width class

position: inherit;
margin-top: var(--spacing-8);
overflow: scroll;
}
.mobile-searchbar > div {
width: 100%;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

@@ -19,14 +19,16 @@ interface SearchInputProps {
}
export const SearchInput = ({onChange, expandable, placeholderText}: SearchInputProps) => {
const [search, setSearch] = useState('')

const inputRef = useRef<HTMLInputElement | null>(null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

try const inputRef = useRef<HTMLInputElement>() - undefined is better for this kind of thing

Copy link
Member Author

Choose a reason for hiding this comment

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

that's a lint error

@@ -44,6 +46,7 @@ export const SearchInput = ({onChange, expandable, placeholderText}: SearchInput
handleSearch(e.currentTarget.value)
}}
value={search}
ref={inputRef}
Copy link
Collaborator

Choose a reason for hiding this comment

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

have you tried just using the autoFocus attribute?

@@ -37,7 +39,9 @@ export const SearchResults = ({results}: {results: SearchResultsProps[]}) => {
<Link key={i} className="search-result" to={result.url}>
<Paper />
<div className="search-result-content">
<h5 className="search-result-title">{result.title}</h5>
<h5 className={['search-result-title', isMobile ? 'xs-bold' : ''].join(' ')}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you do this with a media query, rather than adding clases?

Copy link
Member Author

Choose a reason for hiding this comment

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

How do I add a class to another class with a media query?

Copy link
Collaborator

Choose a reason for hiding this comment

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

you don't :/ This way is fine, but it would be nice avoid explicitly checking the screen size in js, if possible

Copy link
Member Author

@buddy-web3 buddy-web3 Mar 1, 2024

Choose a reason for hiding this comment

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

To resolve your first 2 comments, I would need to put 1 js screen size check for the first one.
And for the second one, a check in a large if loop.
Should I still resolve the first 2?

Copy link
Collaborator

Choose a reason for hiding this comment

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

which comments? I'm not sure I understand

Copy link
Member Author

Choose a reason for hiding this comment

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

The 2 regarding full-width class

Copy link
Collaborator

Choose a reason for hiding this comment

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

wait, so the items in navBar.css aren't for that component? Why aren't they in the appropriate css file?
But no point in adding the classes, then :D

Copy link
Member Author

Choose a reason for hiding this comment

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

Those are for the search results. Same-same, maybe slightly different, but same:)

@buddy-web3 buddy-web3 merged commit 143497c into stampy-redesign Mar 5, 2024
1 check passed
@buddy-web3 buddy-web3 deleted the stampy-redesign-449-reviews branch March 5, 2024 02:17
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