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

Temporary hack to constrain LLP container size. #6138

Merged
merged 1 commit into from
Feb 13, 2018
Merged

Conversation

rxl881
Copy link
Contributor

@rxl881 rxl881 commented Feb 13, 2018

No description provided.

@rxl881 rxl881 merged commit a966cc7 into develop Feb 13, 2018
@turt2live
Copy link
Member

This causes Riot to be broken for everyone with the tag panel enabled (the opposite of #6136 )

@rxl881
Copy link
Contributor Author

rxl881 commented Feb 13, 2018

Thanks @turt2live - I was worried about that happening, but unfortunately am unable to test LLP at the moment (and am not familiar with it). Any suggestions on a temporary fix would be welcomed.

@rxl881
Copy link
Contributor Author

rxl881 commented Feb 13, 2018

I'm currently testing reverting the PRs from earlier to get back to a stable / unbroken state.

@turt2live
Copy link
Member

I'd rather see a permanent fix to resolve #6136 to be honest. In the meantime, I've opened a PR to revert this one: #6139

@rxl881
Copy link
Contributor Author

rxl881 commented Feb 13, 2018

I'd assume that less people currently have LLP enabled (than not), so reverting that PR is just breaking it for more people, ...

@turt2live
Copy link
Member

I'd argue the other way, given /develop is a testing ground. Either way, from an external perspective this isn't okay at all: A temporary fix which just inverts the problem instead of actually fixing the issue is not a fix, not to mention the lack of review on the PR. The review may have been done out of band, and if so, it'd be nice to see that documented somewhere as I have no choice but to assume this was merged without review :(

@rxl881
Copy link
Contributor Author

rxl881 commented Feb 13, 2018

Yup, it's apples and oranges. Very happy to merge your PR (and we're going to have to), but just not sure that it improves the situation that much on it's own.

I'm currently struggling to roll back https://github.com/vector-im/riot-web/pull/6127/files (which I believe to be the main cause of the issues) to a working state.

@rxl881
Copy link
Contributor Author

rxl881 commented Feb 13, 2018

The review may have been done out of band, and if so, it'd be nice to see that documented somewhere as I have no choice but to assume this was merged without review :(

The PR was reviewed out of band with review from @dbkr.

@turt2live
Copy link
Member

255px seems like a better temporary fix for now. I don't know much about the css in that area and some quick experiments failed to produce reasonable results.

With 255px the following happens:

  • For people with the tag panel enabled - the MiddlePanel becomes less cut off and looks more okay
  • For people with the tag panel disabled - the MiddlePanel gets pushed to the right a bit, knocking it off center, but it is readable.

It's not a great solution, but it seems to work well enough until someone can fix it for real.

@ara4n
Copy link
Member

ara4n commented Feb 13, 2018

Either way, from an external perspective this isn't okay at all

I think you may have the wrong end of the stick here. Someone broke develop just before going home; @rxl881 and @dbkr have been interrupting their evenings to try to unbreak it as rapidly as possible.

: A temporary fix which just inverts the problem instead of actually fixing the issue is not a fix

Correct. nobody claimed it was a permenant fix; it was a temporary hack.

not to mention the lack of review on the PR. The review may have been done out of band, and if so, it'd be nice to see that documented somewhere as I have no choice but to assume this was merged without review :(

I reviewed it as did @dbkr (albeit without either of us having access to laptop given we were both out).

In other words: thank you @rxl881 for burning your evening trying to fix someone else's mess - it's very much appreciated.

@turt2live
Copy link
Member

Apologies, I didn't realize it was that broken and people were burning their evenings. From over here it looked like a feature that the LLP got narrower and the bug (#6136) seemed like something that could be fixed tomorrow.

Thank you for your continued dedication, and sorry if I came across harsher than intended.

@rxl881 rxl881 deleted the rxl881/llpFix branch April 3, 2018 21:23
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.

3 participants