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

(fix: QLayout & QHeader): not always sticky when containerized (fix: #11397) #11369

Merged

Conversation

Evertvdw
Copy link
Contributor

When using a containerized layout the 'H/h' and 'F/f' is not respected.
This means a containerized layout behaves different from a normal layout which I don't think is very logical. I did not see any abnormalities when testing it like this.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Documentation
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • It's submitted to the dev branch (or v[X] branch)
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • It's been tested on a Cordova (iOS, Android) app
  • It's been tested on a Electron app
  • Any necessary documentation has been added or updated in the docs (for faster update click on "Suggest an edit on GitHub" at bottom of page) or explained in the PR's description.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

…ky header and footer.

When using a containerized layout the 'H/h' and 'F/f' is not respected.
This means a containerized layout behaves different from a normal layout.
@Evertvdw Evertvdw changed the title (fix: QLayout & QHeader): not always sticky when containerized (fix: QLayout & QHeader): not always sticky when containerized (fix: #11397) Nov 18, 2021
Copy link
Member

@rstoenescu rstoenescu left a comment

Choose a reason for hiding this comment

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

This is on purpose, but honestly I can't remember the reason.

This should be tested on a page with multiple QLayout view configurations (including "funky" ones like "lhh LpR ffr" but not limited to) and on:

  • Desktop Chrome / FF / Safari
  • Mobile browser Chrome / FF / Safari
  • Cordova app Android / iOS
  • Capacitor app Android / iOS
  • PWA (after installing it and launching it): iOS (especially), Android
  • Electron

Cannot merge the PR until these tests are done.

@Evertvdw
Copy link
Contributor Author

@rstoenescu I don't have the resources to check this, I have never done anything with cordova/capacitor/electron. But it seems to me the current behavior is not according to the API. If there is some issue with lowercase h and f with some of these configurations those configurations can just use H and F right? It seems odd to do fixed for all just because it won't work somewhere.

@rstoenescu
Copy link
Member

I understand your concerns, but from a framework perspective we cannot ship features that do not work in every scenario. One scenario is enough to call it a "bug". And for some platforms like iOS (especially!) it's simply not possible to support some features due to the rendering engine (or some other browser implementation relating thing). Sometimes we can take a look at the platform we're running on and decide "we're gonna render this differently for this platform because of X", but this in turn creates unpredictable results for developers... which is not a great experience. Overall, not saying it would be good to check again all the cases and re-asses the situation. Maybe it was IE11 (which is now not supported anymore) or iOS at fault here (I should have documented this at the time of implementation), but now it works again, so we can merge the changes in this PR.

Let's say this QLayout case didn't work on iOS and now it does (with the newer iOS versions). Then discovering iOS fixed stuff so now our own stuff also work is great! But it needs thorough testing as described above. More than above (and I should've mentioned it) is that I'd add a QPageSticky in every of the test cases. Can't merge a PR and "hope for the best". This is not how Quasar reached its performance nor the huge feature set (which should work great together, no matter the modern browser that is running on).

I'm pretty tided up at the moment with other work items, so anyone being able to do the testing that I described and confirming that it works as expected would be of fantastic help to us.

@Evertvdw
Copy link
Contributor Author

I will try and see what I can do regarding testing, help would be appreciated. This is quite a blocking issue for me, as there is no way to do a workaround in my scenario. Only thing I can do at the moment is write a custom Layout component which does the same as QLayout but with this fix.

@rstoenescu rstoenescu merged commit ae2ab94 into quasarframework:dev Dec 9, 2021
@rstoenescu
Copy link
Member

Seems like the culprit is the funky iOS.
So I guess it's ok to maintain the current limitation only for iOS.

rstoenescu added a commit that referenced this pull request Dec 9, 2021
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