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

Some viewport profiles in getViewportProfile are not determined correctly #691

Open
TAKARA328 opened this issue Jun 6, 2021 · 6 comments
Labels

Comments

@TAKARA328
Copy link

TAKARA328 commented Jun 6, 2021

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Other... Please describe:

Expected Behavior

getViewportProfile() returns the correct viewport profile.

Current Behavior

When I tried it with the Alexa simulator, I got the following results.

No. Alexa Simulator getViewportProfile()
1 Hub Round Small HUB-ROUND-SMALL ✔︎
2 Hub Landscape Small HUB-LANDSCAPE-SMALL ✔︎
3 Hub Landscape Medium MOBILE-LANDSCAPE-MEDIUM
4 Hub Landscape Large HUB-LANDSCAPE-LARGE ✔︎
5 TV Landscape Extra Large TV-LANDSCAPE-XLARGE ✔︎
6 Mobile Small UNKNOWN-VIEWPORT-PROFILE
7 Mobile Medium HUB-LANDSCAPE-MEDIUM
8 Mobile Large MOBILE-LANDSCAPE-MEDIUM

Possible Solution

The cause is that there is an error in the conditional expression.
Modify the conditional expression and use mode.
Add the condition of MOBILE-LANDSCAPE-LARGE and MOBILE-PORTRAIT-LARGE.

getViewportProfile() Changes
HUB-LANDSCAPE-MEDIUM - viewportDpiGroup to 'MEDIUM'
MOBILE-LANDSCAPE-SMALL
MOBILE-PORTRAIT-SMALL
- viewportDpiGroup to 'LOW'
MOBILE-LANDSCAPE-MEDIUM - viewportDpiGroup to 'LOW'
- pixelHeightSizeGroup to 'XSMALL'
MOBILE-PORTRAIT-MEDIUM - pixelWidthSizeGroup to 'LOW'
- pixelHeightSizeGroup to 'XSMALL'
MOBILE-LANDSCAPE-LARGE
MOBILE-PORTRAIT-LARGE
- New addition
        if (mode === 'HUB') {
            if (shape === 'ROUND'
                && viewportOrientation === 'EQUAL'
                && viewportDpiGroup === 'LOW'
                      :
                      :

Modify the test case of ViewportUtils.spec.ts.

For example HUB-LANDSCAPE-MEDIUM
Added mode. Changed dpi from 160 to 213.

        requestEnvelope.context.Viewport.shape = 'RECTANGLE';
        requestEnvelope.context.Viewport.mode = 'HUB';
        requestEnvelope.context.Viewport.currentPixelHeight = 600;
        requestEnvelope.context.Viewport.currentPixelWidth = 960;
        requestEnvelope.context.Viewport.dpi = 213;
        expect(getViewportProfile(requestEnvelope)).eq('HUB-LANDSCAPE-MEDIUM');

Steps to Reproduce (for bugs)

Context

Your Environment

  • ASK SDK for Node.js used: 2.10.2 (ask-sdk-core)
  • Operating System and version: Windows 10

Node.js and NPM Info

  • Node.js version used for development: 14.17.0
  • NPM version used for development: 7.16.0
TAKARA328 added a commit to TAKARA328/alexa-skills-kit-sdk-for-nodejs that referenced this issue Jun 6, 2021
@ShenChen93
Copy link
Contributor

Hi @TAKARA328

Thanks for posting this issue. It turns out this util function is not up to date with latest viewportProfile table. We will update this util function ASAP after we confirm with viewportProfile team.

Thanks,
Shen

@ShenChen93 ShenChen93 added the bug label Jun 7, 2021
@TAKARA328
Copy link
Author

TAKARA328 commented Jun 7, 2021

Hi @ShenChen-Amazon

Thanks for good reply. I'm looking forward to it fixed.

Thanks,
TAKARA

TAKARA328 added a commit to TAKARA328/alexa-skills-kit-sdk-for-nodejs that referenced this issue Jun 8, 2021
TAKARA328 added a commit to TAKARA328/alexa-skills-kit-sdk-for-nodejs that referenced this issue Jun 8, 2021
@aknad
Copy link

aknad commented Oct 14, 2021

Hi @ShenChen-Amazon,
do you have an update about this issue?
thanks

@aknad
Copy link

aknad commented Nov 9, 2021

the viewports of the echo show 15 are also missing

@rahulawl
Copy link
Contributor

Is this issue/feature-request still relevant?
We are working on prioritization of relevant issues and cleanup of rest. If we don’t hear back in 2 weeks, we will assume that the issue is not relevant and we will close it.

@TAKARA328
Copy link
Author

I would like you to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants