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.

Sign up for GitHub

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

Jump to bottom

42678 adaptive image resolution #710

Merged
merged 32 commits into from
Jun 18, 2024
Merged

42678 adaptive image resolution #710

merged 32 commits into from
Jun 18, 2024

Conversation

yeneastgate
Copy link
Contributor

related to #675
changed log:
Update adaptive image resolution

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 28, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 9109745900

Details

  • 20 of 20 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 79.681%
Totals Coverage Status
Change from base Build 9109632295: 0.04%
Covered Lines: 8353
Relevant Lines: 10483
💛 - Coveralls

@fredericalpers fredericalpers added this to the v4.19 milestone Jan 8, 2024
@fredericalpers fredericalpers linked an issue Jan 8, 2024 that may be closed by this pull request
Adaptive image resolution #675
Closed
4 tasks
@fredericalpers fredericalpers added the QA Issue or Pull request that is in review label Jan 8, 2024
@fredericalpers fredericalpers added feature New feature component: property list Issue, Pull Request or Discussion related to property lists component: property detail page Issue, Pull Request or Discussion related to property detail page labels Feb 20, 2024
@fredericalpers fredericalpers modified the milestones: v4.19, v4.20 Feb 23, 2024
pospisk

This comment was marked as outdated.

Sign in to view

@github-actions GitHub Actions
Copy link

github-actions bot commented May 7, 2024

Steps to install the approved version:

  1. Download onoffice-4.19-5-g8ad4d125-please-unpack.zip from https://github.com/onOffice-Web-Org/oo-wp-plugin/actions/runs/8984018660.
  2. Unpack the downloaded file to get another .zip file.
  3. Upload that inner .zip file to WordPress.

@fredericalpers fredericalpers added the deploy test Triggers event to create test version label May 8, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented May 8, 2024

Steps to install the approved version:

  1. Download onoffice-4.19-20-gc6d1c90f-please-unpack.zip from https://github.com/onOffice-Web-Org/oo-wp-plugin/actions/runs/8998775163.
  2. Unpack the downloaded file to get another .zip file.
  3. Upload that inner .zip file to WordPress.

@andernath
fix unit test
77456a8 
#4246743
andernath added 4 commits May 27, 2024 07:19
@andernath
fix unit test
0ee616f 
#4246743
@andernath
fix unit test
d783150 
#4246743
@andernath
fix unit test
65f7c97 
#4246743
@andernath
fix unit test
c527e1c 
#4246743
@fredericalpers fredericalpers added deploy test Triggers event to create test version and removed deploy test Triggers event to create test version labels May 27, 2024
@github-actions GitHub Actions
Copy link

Steps to install the approved version:

  1. Download onoffice-4.19-32-gd0e44479-please-unpack.zip from https://github.com/onOffice-Web-Org/oo-wp-plugin/actions/runs/9252734063.
  2. Unpack the downloaded file to get another .zip file.
  3. Upload that inner .zip file to WordPress.

@fredericalpers fredericalpers added deploy test Triggers event to create test version and removed deploy test Triggers event to create test version labels May 29, 2024
@github-actions GitHub Actions
Copy link

Steps to install the approved version:

  1. Download onoffice-4.19-35-g5419dd89-please-unpack.zip from https://github.com/onOffice-Web-Org/oo-wp-plugin/actions/runs/9282050910.
  2. Unpack the downloaded file to get another .zip file.
  3. Upload that inner .zip file to WordPress.

@fredericalpers fredericalpers requested a review from pospisk June 5, 2024 07:23
pospisk

This comment was marked as outdated.

Sign in to view

@github-actions GitHub Actions
Copy link

Steps to install the approved version:

  1. Download onoffice-4.19-40-gb1d807da-please-unpack.zip from https://github.com/onOffice-Web-Org/oo-wp-plugin/actions/runs/9483409096.
  2. Unpack the downloaded file to get another .zip file.
  3. Upload that inner .zip file to WordPress.

andernath
andernath reviewed Jun 17, 2024
View reviewed changes
Copy link
Contributor

@andernath andernath left a comment

Choose a reason for hiding this comment

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

@pospisk please take a look

templates.dist/estate/default.php Outdated
$image_width_lg = 250;
$image_width_xl = 250;
$image_width_xxl = 250;
$image_width_xxxl = 250;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wie ermittelst du die maximale Breite für ein Bild?
Ich bin hier von einem fluid container ausgegangen, in dem der Inhalt liegt und habe mir die Breiten bei einem dreiteiligen Layout angeschaut.

Hast du hier vielleicht Höhe und Breite vertauscht?
Ist das bei detail und similar estate eventuell der selber Grund?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ich hatte das mit der W3PM Neve-Child Instanz getestet (das Ticket weist auch in diese Richtung), aber gerade ist mir klar geworden, dass die Größen hier leider teilweise irrelevant sind, weil jeder Benutzer ein anderes Theme mit unterschiedlichen Inhaltsbreiten verwenden kann. Das führt dazu, dass wir keine „richtigen“ Größen für das Standard-Template liefern können, da die Seiten immer unterschiedlich sein werden.

Die Breiten hatte ich manuell anhand der Elementbreite für die jeweiligen Breakpoints getestet:

Zum Beispiel bei 576px - 768px (sm) ist der Mittelwert des Breakpoints 672px, und dafür hatte ich 350px genommen, weil, wenn 2 Spalten angezeigt werden, die 300px & 300px breit sind, bei 672px dann ein Bild 50px herunterskaliert werden muss, und bei 767px ist es dann 350px breit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Genau deswegen habe ich versucht eine sinnvolle größtmögliche Breite bei einem dreiteiligen Layout zu wählen.
Hast du einen besseren Vorschlag, als dieses Vorgehen?
Wenn das Styling customized wurde, werden wahrscheinlich eigene Templates verwendet und da können die Breakpoint Größen individuell angepasst werden.

Ich werde den Commit rückgängig machen.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nein, deine Bildgrößen können dann bleiben. Nur bei xs verwende bitte eine kleinere Größe als 545px.
Lighthouse Mobile testet mit 412px device width

@andernath andernath merged commit 901458d into master Jun 18, 2024
3 checks passed
@andernath andernath deleted the 42678-adaptive-image-resolution branch June 18, 2024 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andernath andernath andernath left review comments

@pospisk pospisk pospisk left review comments

Assignees
No one assigned
Labels
component: property detail page Issue, Pull Request or Discussion related to property detail page component: property list Issue, Pull Request or Discussion related to property lists deploy test Triggers event to create test version feature New feature QA Issue or Pull request that is in review
Projects
None yet
Milestone
v4.20
Development

Successfully merging this pull request may close these issues.

Adaptive image resolution
4 participants
@yeneastgate @pospisk @andernath @fredericalpers