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

Navigation position style fixes [storage] #2193

Merged
merged 4 commits into from
Jun 21, 2022
Merged

Conversation

tanmoyAtb
Copy link
Contributor

closes #2087


Submission checklist:

Layout

  • Change looks good in the desktop web ui
  • Change looks good in the mobile web ui

@render
Copy link

render bot commented Jun 15, 2022

@render
Copy link

render bot commented Jun 15, 2022

@render
Copy link

render bot commented Jun 15, 2022

@github-actions github-actions bot added the Type: Bug Fix Added to PRs if they are addressing a bug label Jun 15, 2022
@tanmoyAtb tanmoyAtb marked this pull request as ready for review June 16, 2022 12:23
Copy link
Contributor

@juans-chainsafe juans-chainsafe left a comment

Choose a reason for hiding this comment

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

Looking good! but I have a problem, only in mobile of course, using Chrome or Metamask app:
In CIDs table I only see the columns "Name", "Cid" and the 3 dots. I can't move left or right to see other columns ("Created", "Size", "Status")
IMG_BB12982AE042-1

@tanmoyAtb
Copy link
Contributor Author

Looking good! but I have a problem, only in mobile of course, using Chrome or Metamask app: In CIDs table I only see the columns "Name", "Cid" and the 3 dots. I can't move left or right to see other columns ("Created", "Size", "Status") IMG_BB12982AE042-1

This was very intentional actually. If you have multiple columns in the table, the alignment of the whole page breaks and looks odd.
I wanted to add a quick fix to keep the navigation usable and I don't think anyone would look for CID information on mobile, therefore omitted the columns.

Copy link
Contributor

@juans-chainsafe juans-chainsafe left a comment

Choose a reason for hiding this comment

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

@tanmoyAtb well, yes, make sense to only have those columns in mobile, but just in case I want @asnaith opinion on that to be sure!

👍 I agree with Tanmoy's rationale, it's not worth the time investment in making the view support all columns as are expecting very low usage of Storage UI on mobile devices.

@asnaith
Copy link
Member

asnaith commented Jun 16, 2022

@tanmoyAtb well, yes, make sense to only have those columns in mobile, but just in case I want @asnaith opinion on that to be sure!

👍 I agree with Tanmoy's rationale, it's not worth the time investment in making the view support all columns as are expecting very low usage of Storage UI on mobile devices.

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Just a nit, the code is looking good otherwise

packages/storage-ui/src/Components/Pages/CidsPage.tsx Outdated Show resolved Hide resolved
packages/storage-ui/src/Components/Elements/CidRow.tsx Outdated Show resolved Hide resolved
tanmoyAtb and others added 2 commits June 20, 2022 21:53
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
@tanmoyAtb
Copy link
Contributor Author

Just a nit, the code is looking good otherwise

Updates are in.

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Code looks good

@Tbaut Tbaut merged commit c2b71c8 into dev Jun 21, 2022
@Tbaut Tbaut deleted the fix/nav-positioning-2087 branch June 21, 2022 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Fix Added to PRs if they are addressing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Storage] Element positioning issue on slide-out navigation (Android)
4 participants