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

Create Mobile Editor Endpoint #1455

Merged
merged 8 commits into from
Jun 15, 2020

Conversation

ghalestrilo
Copy link
Collaborator

Part of the Mobile Web Editor project.

This PR creates a /mobile endpoint, rendering a new screen, which will be used for the development of the mobile web editor. This screen is enabled/disabled through the MOBILE_ENABLED environment variable.
Currently it's just groundwork getting done. Future PRs will implement its design and behavior.

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 ghalestrilo requested a review from catarak June 12, 2020 19:17
// const isMobile = () => window.innerWidth <= 760;
// const IDEView = isMobile() && !ignoreMobile() ? IDEViewMobileScreen : IDEViewScreen;

// How to use URL as a prop?
Copy link
Member

Choose a reason for hiding this comment

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

@ghalestrilo What is it you want to use the URL for? Maybe I can help?

Copy link
Member

Choose a reason for hiding this comment

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

@andrewn the idea here is to have a url parameter to render the desktop view even if the user is on a small screen.

Copy link
Member

Choose a reason for hiding this comment

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

also @ghalestrilo if possible, i'd prefer to not merge in commented out code, so would it be possible to remove this for now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi guys, sorry for the delay.
Absolutely, will update this right now. @andrewn I had coded a way to bypass the /mobile view through a URL parameter, using window.location, and @catarak suggested we instead used the query parameter as a prop.
I wrote the comment because I didn't know how to do it at the moment, but I think following the react-router documentation will be simple enough :)

Copy link
Member

Choose a reason for hiding this comment

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

I see. Yeah, there's no way to specify it in the Route and react-router doesn't automatically parse it ASFAIK.

I usually:

  1. Get the location object passed into the mounted component (or the withRouter HoC, or the useLocation hook).
  2. Pass location.search into the browser's native URLSearchParams helper

What I've used in the past, is fetching the querystring from location.search


const Header = styled.div`
width: 100%;
background-color: ${background} !important;
Copy link
Member

Choose a reason for hiding this comment

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

do you need to use !important here? i'd prefer not to use it ever unless absolutely necessary (e.g. overriding styles from another library).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point, will change this

@catarak
Copy link
Member

catarak commented Jun 15, 2020

I know this is in progress, but I made a couple of code clean up suggestions! Otherwise this is great.

@catarak catarak merged commit b8ba3b9 into processing:develop Jun 15, 2020
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.

None yet

3 participants