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

Feature/mobile examples #1528

Merged
merged 68 commits into from
Aug 17, 2020

Conversation

ghalestrilo
Copy link
Collaborator

Requires #1513

Creates mobile My Stuff / Examples views, through the <MobileDashboardView />

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • is from a uniquely-named feature branch and has been rebased on top of the latest master. (If I was asked to make more changes, I have made sure to rebase onto master then too)
  • is descriptively named and links to an issue number, i.e. Fixes #123

@ghalestrilo
Copy link
Collaborator Author

This PR implements a bug where the navigation from <IDEView /> to other views stops working.

  • Popups and modals work fine.
  • Simulating clicks from the console also do not work (aka $("a[href="/login"]).click())
  • It doesn't seem to be related to the custom useAsModal / useModalBehavior hooks, because:
    1. It doesn't seem to happen on feature/mobile-header-dropdown-menu, which implements them
    2. Commenting / dummying them out does not fix the bug
  • It seems to involve routes.jsx / server.routes.js, no idea how, though.


// Mobile Routes
if (process.env.MOBILE_ENABLED) {
router.get('/mobile', (req, res) => res.send(renderIndex()));
Copy link
Member

Choose a reason for hiding this comment

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

If these routes are the same as the desktop routes above, one way to avoid duplication would be to mount them again under the /mobile namespace in server.js:

// this is supposed to be TEMPORARY -- until i figure out
// isomorphic rendering
app.use('/', serverRoutes);

if (process.env.MOBILE_ENABLED) {
  app.use('/mobile’, serverRoutes);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wow this is amazing, I didn't know this was possible, thanks andrew!

@catarak
Copy link
Member

catarak commented Aug 3, 2020

IMG_2954
This is how it looks right now on my phone. Some thoughts:

  • The "Sketch" heading and sketch titles don't align. I think the sketch heading needs less padding.
  • The table itself is overflowing off the right side of the screen
  • I think the table text overall could be smaller, the same font size as the headings
  • If it still feels cramped, you could possibly put the dates on a different line from the sketch title (see CodePen dashboard for reference:
    IMG_2955
  • It could make sense to remove the times from the "created" and "updated" dates, for more room.

@ghalestrilo
Copy link
Collaborator Author

The navigation bug has been fixed. The navigation was being prevented because #warnIfUnsavedChanges (IDEView.jsx:54) was returning false by default. It has been updated to return true

@ghalestrilo ghalestrilo self-assigned this Aug 3, 2020
@ghalestrilo
Copy link
Collaborator Author

Nice! I made the changes you suggested:

Screenshot from 2020-08-11 16-58-35
Screenshot from 2020-08-11 16-58-50

the button to the far-left can't be much thinner without practically merging with the left cell (visually), because there isn't enough content, so it looks a bit weird and needlessly right-heavy

@catarak
Copy link
Member

catarak commented Aug 11, 2020

Awesome 😄

I was able to get just the header to center all of the items by selecting the <tr> and <td> just in the <thead>:

.<generatedclass> thead tr {
      grid-template-columns: 33% 33% 33% 0fr;
}

.<generated class> thead td {
    display: flex;
    justify-content: center;
}

Which looks like this:
Screen Shot 2020-08-11 at 4 38 15 PM

@catarak
Copy link
Member

catarak commented Aug 11, 2020

Then, I would also remove all of the padding-left on the tbody td selector, which is causing the dates to not be aligned with the sketch titles.

Screen Shot 2020-08-11 at 4 47 23 PM

@catarak
Copy link
Member

catarak commented Aug 11, 2020

The other changes you made are perfect 🌈

@ghalestrilo
Copy link
Collaborator Author

Great! I managed to get the header the way you suggested by fiddling with the grid layout. About the 12px left-padding: I think it's not what caused the misalignment - I think it was the cell's justify-self. In fact, removing it moves the text too far to the left 🤦🏻 Right now it looks like this:

Screenshot from 2020-08-11 18-33-31

If the problem persists on your device, can I ask you to take a look into it? I tested a couple different resolutions and browsers here but cannot reproduce

@ghalestrilo ghalestrilo mentioned this pull request Aug 12, 2020
6 tasks
@catarak
Copy link
Member

catarak commented Aug 12, 2020

This is super close to being done! Just a couple more things. I made a couple of CSS adjustments, and then also:

  • The CollectionList is missing a default sort order
  • The AssetList items need labels for the size and sketch they belong to. Because there's no sorting/filtering on this list at the moment, maybe we could even remove the headers because they don't make any sense?

@ghalestrilo
Copy link
Collaborator Author

I hid the header on the assets list, but I don't know what to do about the collections list: it has 4 fields, and fitting them all on the screen ends up looking crammed. Sorry about not catching that earlier!

Screenshot from 2020-08-13 11-02-42

@ghalestrilo
Copy link
Collaborator Author

I made the adjustments you suggested to better fit text on the header. I think this is ready to merge :)
Give me your thoughts

@catarak
Copy link
Member

catarak commented Aug 17, 2020

I don't want to leave this PR open any longer so I think this is ready to merge! A couple of small things that I think could go in another PR:
Screen Shot 2020-08-17 at 2 58 33 PM

  • In the Asset List, I think the "Size" and "Sketch" could be labelled (i.e. prefixed with "Size: " and "Sketch: ")
  • I also noticed that the table rows are alternating colors in this list. Since they have the border/shadows I think maybe this should be removed and match the Sketch List/Collection List. What do you think?

@ghalestrilo ghalestrilo merged commit 127660a into processing:develop Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Mobile UI
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

3 participants