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

Add media queries and mobile viewport #12142

Closed
wants to merge 1 commit into from

Conversation

daleharvey
Copy link
Contributor

As discussed in #9814

I made an additional PR to matrix-react-sdk @ matrix-org/matrix-react-sdk#3991

@daleharvey
Copy link
Contributor Author

Copying the comment over since guessing we want to keep most discussion here?

So there is a lot of work to do here, I wanted to put up the minimal patch that gets the UI usable on mobile and touches the least amount of things as possible so we can discuss the overall approach without too big a patch.

It looks like:

https://i.imgur.com/Z8TU0VE.png
https://i.imgur.com/gvVVlJu.png
https://i.imgur.com/jSMuqAN.png

I feel like we probably dont want to be repeating this media query so often so not sure if we want to put this all in a dedicated mobile-overrides.scss ?

@daleharvey
Copy link
Contributor Author

Signed-off-by: Dale Harvey [email protected]

@turt2live turt2live requested a review from a team January 30, 2020 10:51
@jryans
Copy link
Collaborator

jryans commented Jan 30, 2020

Thanks for the contribution! 😄 As mentioned in few places, we're currently in crunch mode for FOSDEM prep, but we'll take a look at this next week.

@jryans jryans added the Z-Community-PR Issue is solved by a community member's PR label Feb 3, 2020
@jryans
Copy link
Collaborator

jryans commented Feb 6, 2020

Before moving forward here, I'd like to discuss with the Riot core team on our support level for mobile web. I'll keep you updated, and sorry for the delays.

@turt2live turt2live removed the request for review from a team February 6, 2020 13:49
@turt2live
Copy link
Member

(removing this from the github review queue while that happens)

@jryans
Copy link
Collaborator

jryans commented Feb 14, 2020

Some progress to report here! #12377 intends to update our supported environments for Riot. Assuming that moves forward and merges, we'll be ready to pick this up for review.

@jryans
Copy link
Collaborator

jryans commented Feb 17, 2020

The supported environments set has been updated, and so this can now proceed to review.

@jryans jryans removed the X-Blocked label Feb 17, 2020
@jryans jryans requested a review from a team February 17, 2020 12:44
@turt2live turt2live removed the request for review from a team February 18, 2020 21:06
@jryans jryans requested a review from a team February 18, 2020 23:30
@jryans
Copy link
Collaborator

jryans commented Feb 18, 2020

To clarify for reviewers, these are ready for anyone to review. #9814 is not on the workflow board itself since this set is only an initial step.

Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Thanks for your patience with this!

Overall, I think this looks reasonable. A few small things to tweak before merging.

@@ -158,6 +158,16 @@
display: none;
}

@media only screen and (max-width : 480px) {
.mx_ButtonRow {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use 4 space indent.

@@ -158,6 +158,16 @@
display: none;
}

@media only screen and (max-width : 480px) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the space before the colon.

@t3chguy
Copy link
Member

t3chguy commented Mar 29, 2020

Hello @daleharvey, thank you for your contribution a while back, Any chance you could just do the final small tweaks to get it over the line? If not then one of the core team may do so as this would be a nice thing to land.

Thanks

@@ -158,6 +158,16 @@
display: none;
}

@media only screen and (max-width : 480px) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@media only screen and (max-width : 480px) {
@media only screen and (max-width: 480px) {

Comment on lines +162 to +169
.mx_ButtonRow {
flex-direction: column;
}

.mx_ButtonRow > * {
margin: 0 0 10px 0;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.mx_ButtonRow {
flex-direction: column;
}
.mx_ButtonRow > * {
margin: 0 0 10px 0;
}
}
.mx_ButtonRow {
flex-direction: column;
}
.mx_ButtonRow > * {
margin: 0 0 10px 0;
}
}

@TitanNano
Copy link
Contributor

@jryans @t3chguy I made the requested changes. #13818

turt2live added a commit that referenced this pull request May 27, 2020
 Add media queries and mobile viewport (#12142)
@turt2live turt2live closed this May 27, 2020
t3chguy pushed a commit that referenced this pull request Oct 17, 2024
* Enable redirected media by default

See matrix-org/matrix-js-sdk#4007

* Update the tests

* Update tests

* Update end-to-end tests too

* Update linkifier to use Media customisation endpoint

* Fix tests for new js-sdk behaviour

* Fix unchanged file

* Fix linkifier

* Fix lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants