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

Pro 6518 mobile preview #4720

Open
wants to merge 36 commits into
base: main
Choose a base branch
from
Open

Pro 6518 mobile preview #4720

wants to merge 36 commits into from

Conversation

haroun
Copy link
Contributor

@haroun haroun commented Sep 9, 2024

Summary

Add mobile preview feature to the admin UI
image

What are the specific steps to test this change?

https://preview.a3.apos.dev/

or

  1. Enable mobile preview using asset module devicePreviewMode option in your project
     // module/@apostrophecms/asset/index.js
     module.exports = {
       options: {
         devicePreviewMode: {
           enable: true,
           debug: true,
           resizable: false,
           screens: {
             desktop: {
               label: 'apostrophe:devicePreviewDesktop',
               width: '1500px',
               height: '900px',
               icon: 'monitor-icon'
             },
             tablet: {
               label: 'apostrophe:devicePreviewTablet',
               width: '1024px',
               height: '768px',
               icon: 'tablet-icon'
             },
             mobile: {
               label: 'apostrophe:devicePreviewMobile',
               width: '480px',
               height: '1000px',
               icon: 'cellphone-icon'
             }
           }
         },
         // Transform method used on media feature
         // Can be either:
         // - (mediaFeature) => { return mediaFeature; }
         // - null
         transform: null
       }
     };
  2. Check the display (you can add some media queries to your asset to test the rendering)
    // to put in an scss file or a vue component
    @media (max-width: 1400px) {
      [data-apos-refreshable] main h1 {
        background-color: gold;
      }
    }
    
    @media (max-width: 1200px) {
      [data-apos-refreshable] main h1 {
        background-color: green;
      }
    }
    
    @media (max-width: 800px) {
      [data-apos-refreshable] main h1 {
        background-color: blue;
      }
    }
    
    @media (max-width: 600px) {
      [data-apos-refreshable] main h1 {
        background-color: magenta;
      }
    }
    
    @media (max-width: 400px) {
      [data-apos-refreshable] main h1 {
        background-color: red;
      }
    }

What kind of change does this PR introduce?

(Check at least one)

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • Build-related changes
  • Other

Make sure the PR fulfills these requirements:

  • It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
  • The changelog is updated
  • Related documentation has been updated
  • Related tests have been updated

If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

@haroun haroun self-assigned this Sep 9, 2024
Copy link

linear bot commented Sep 9, 2024

* main:
  PRO-6578: auto import of inline images (#4723)
  Inline array improvements (#4671)
  take credit for the fix as well as the "change" (#4725)
  PRO-6576 fix default values for object fields in widgets and generally unify newInstance implementations (#4722)
sourceMap: true
}
}
mediaToContainerQueriesLoader,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@haroun haroun marked this pull request as ready for review September 12, 2024 17:02
Copy link
Member

@stuartromanek stuartromanek left a comment

Choose a reason for hiding this comment

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

Other stuff:

  • On first activation the canvas has no height. I think we’re supposed to simulate actual device heights, but in lieu of not having a height we should set the height to something like 100vh - (admin bar height + canvas margin)
  • A small setting label should be above the upper left corner of the canvas
image
  • For now, disable resize
  • There needs to be style differentiation between the resized site and the layer the canvas ‘sits on’. The layer behind the canvas should be a light gray while the canvas should be styles as they define. What if they style the ? Currently the refreshable container doesn’t have any style applied and if you style the body you get this. This seems like a big hole. If you style the body things currently look like this
image

See https://www.sketch.com/s/fbfcda4a-08a4-4c8d-ab88-fdad7532217e/a/ZVqLgA7

  • Moving from page to page should preserve your preview setting. Currently it does not.
  • withdrawn style comment

</script>
<style lang="scss" scoped>
.apos-admin-bar__device-preview-mode {
margin-left: 22px;
Copy link
Member

Choose a reason for hiding this comment

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

round to 20px (base * 2)


.apos-admin-bar__device-preview-mode-button {
& + & {
margin-left: 6px;
Copy link
Member

Choose a reason for hiding this comment

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

Round to 5px (base / 2) ..
Also, this UI seems like flex + gap would be good

// Breakpoints
breakpoints: {
// If we can resize the preview container?
resizable: false,
Copy link
Member

Choose a reason for hiding this comment

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

move this up a level?

// Screen list with icons
// For adding icons, please refer to the icons documentation
// https://docs.apostrophecms.org/reference/module-api/module-overview.html#icons
screens: {
Copy link
Member

Choose a reason for hiding this comment

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

breakpoints = screens?

Comment on lines 65 to 69
desktop: {
label: 'apostrophe:devicePreviewDesktop',
minWidth: 1024,
icon: 'monitor-icon'
},
Copy link
Member

Choose a reason for hiding this comment

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

These need to be able to support heights?

@@ -0,0 +1,71 @@
module.exports = function (source) {
Copy link
Member

Choose a reason for hiding this comment

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

The road to hell is paved with good intentions

Copy link
Member

@boutell boutell left a comment

Choose a reason for hiding this comment

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

Stu covered the territory well.

Device heights are part of the tech design and should be implemented, the view should be scaled so that you can see the entire device showing its first page in terms of height, and its scrollbar should be based on its native height. People will want to know what is "above the fold" (might seem a tired concept, but valid for stuff like logos that had better fit on the first screen for instance)

* main:
  hotfix 4.7.1 (#4733)
  PRO-6591: parked fields overrides (#4732)
  do not let a page become a child of itself (#4726)
  adds focus states for file input and media lib uploader (#4729)
  add <use> tag to DOMPurify allow list when sanitizing uploaded SVGS (#4727)
@haroun
Copy link
Contributor Author

haroun commented Sep 23, 2024

@stuartromanek still working on

Moving from page to page should preserve your preview setting. Currently it does not.

the rest is there

@stuartromanek
Copy link
Member

@haroun looked at this again, really shaping up.

I committed a few style adjustments rather than go back and forth here. Be sure to pull!

I see the page-to-page stuff is still being worked out, no worries there

Two main issues I see

image

The content here runs out of the preview container but currently there is no ability to scroll. Fix here is fairly obvious, the container should scroll. If possible, disable scroll on the X axis. Naively

overflow-x: clip;
overflow-y: scroll;

The second issue being that the body styling issue is still not solved. Left has the site's real body style, right its being overridden to provide the canvas background.

image

height: screen.height
}
},
shortcut: `P,${index}`
Copy link
Contributor

@ETLaurent ETLaurent Sep 24, 2024

Choose a reason for hiding this comment

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

Suggested change
shortcut: `P,${index}`
shortcut: `P ${index}`

seems more UX-friendly to me

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.

4 participants