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

Marko profile page header responsiveness ( userprofile img, searchbox, connectwallet) #151

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

Conversation

markosavicmidusa
Copy link

@markosavicmidusa markosavicmidusa commented Mar 6, 2022

Pull Request Template

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A
  • Test B

Test Configuration(if any):

  • Firmware version:
  • Hardware:
  • Toolchain:
  • SDK

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings
    -Edited pull request

@vercel
Copy link

vercel bot commented Mar 6, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/minorityprogrammers/mpa-webapp/Hg2BAMTUazsZm7qDo4X8APtigoCK
✅ Preview: https://mpa-webapp-git-marko-profile-page-re-fa2046-minorityprogrammers.vercel.app

…ll be called...same functionallity will be called when user clicks with cursor on search icon...(functionallity is added for mobile size screens too)
Copy link
Collaborator

@Ebube111 Ebube111 left a comment

Choose a reason for hiding this comment

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

image

In my opinion, the search border color should have a normal color or a dim or different color, then when on focus it should have that purple border color (try adding a little bit of transition as well) , or what do you think??
Also, the font looks too large for a search bar of that width
the color of the placeholder looks like text already. try making it gray or something dim

Comment on lines 239 to 240
<div style={{border:"1px solid red"}} className="nav__mobile-profile tw:z-10">
<div style={{border:"1px solid red"}} className="nav__mobile-img">
Copy link
Collaborator

Choose a reason for hiding this comment

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

since you're just adding a border and not manipulating the DOM, why not add it to the className instead, or better still use tailwind, so we can stick to classNames. for consistency. WDYT ??

Copy link
Author

Choose a reason for hiding this comment

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

solved

Copy link
Author

Choose a reason for hiding this comment

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

these are my oriantation borders, before I was interducing tailwind...I forgot to delete them

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool

Comment on lines 55 to 57
/*router.push({
pathname: router.pathname,
query: { _q: encodeURI(e.target.value) },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clean up, if commented out code would not be used in the future

Copy link
Author

Choose a reason for hiding this comment

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

solved

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool

@markosavicmidusa
Copy link
Author

image

In my opinion, the search border color should have a normal color or a dim or different color, then when on focus it should have that purple border color (try adding a little bit of transition as well) , or what do you think?? Also, the font looks too large for a search bar of that width the color of the placeholder looks like text already. try making it gray or something dim

I think I solved these points...This is a first time using tailwind...
So there was few points where I lost lot of time... and maybe I think only these points were left - unhandled by developer who worked on it ...Example : I need tw-mt-7 (but it doesn't work)...And what is very strange tw-mt-6 , and tw-mt-8 are working....For some reason they don't work, also the some other tw-classes too...
But nevermind...hope for good reviews

@Ebube111
Copy link
Collaborator

image
In my opinion, the search border color should have a normal color or a dim or different color, then when on focus it should have that purple border color (try adding a little bit of transition as well) , or what do you think?? Also, the font looks too large for a search bar of that width the color of the placeholder looks like text already. try making it gray or something dim

I think I solved these points...This is a first time using tailwind... So there was few points where I lost lot of time... and maybe I think only these points were left - unhandled by developer who worked on it ...Example : I need tw-mt-7 (but it doesn't work)...And what is very strange tw-mt-6 , and tw-mt-8 are working....For some reason they don't work, also the some other tw-classes too... But nevermind...hope for good reviews

Yeah thats how tailwind is- tailwind deals with only even-numbers in terms of padding, margin and anything that has to do with numbers, there are no odd numbers

@markosavicmidusa
Copy link
Author

Finished:
Centerize connect wallet on mobile screens
Bringing back the old code for search input logic

@markosavicmidusa
Copy link
Author

Commented the search box-on mobile screens

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