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

[2.x] Change Inertia Stubs to Composition API #1001

Conversation

xiCO2k
Copy link
Contributor

@xiCO2k xiCO2k commented Mar 15, 2022

This PR introduces the Composition API to the stubs of Inertia.

I will be using Jetstream with Inertia for a small project and I will be upgrading the inertia stubs to Composition API on my own project, so I will be able to PR all the changes from options API to composition API.

Do you guys think it will be a good option to have by default?

Thanks.

@xiCO2k xiCO2k changed the title change Inertia login stub to composition API [2.x] Change Inertia login stub to composition API Mar 15, 2022
@xiCO2k xiCO2k changed the title [2.x] Change Inertia login stub to composition API [2.x] Change Inertia login stub to Composition API Mar 15, 2022
@taylorotwell
Copy link
Member

taylorotwell commented Mar 15, 2022

I think it's a good idea but we would want to do the whole thing at once instead of just one page. Breeze is already using Composition API with script setup so it makes sense to move Jetstream to it as well.

In Breeze I believe we also moved the script section above template.

@taylorotwell taylorotwell marked this pull request as draft March 15, 2022 18:34
@xiCO2k
Copy link
Contributor Author

xiCO2k commented Mar 15, 2022

Will do that, and check breeze code to match the same style.

Thanks

@xiCO2k xiCO2k force-pushed the feat/update-inertia-login-stub-to-composition-api branch from 51c9610 to e8d2720 Compare March 16, 2022 02:54
@xiCO2k xiCO2k changed the title [2.x] Change Inertia login stub to Composition API [2.x] Change Inertia Stubs to Composition API Mar 16, 2022
@xiCO2k xiCO2k marked this pull request as ready for review March 16, 2022 02:56
@xiCO2k
Copy link
Contributor Author

xiCO2k commented Mar 16, 2022

Should be ready.

@taylorotwell
Copy link
Member

@xiCO2k One small bug I have noticed so far is if you attempt to enable two factor authentication (or any other action which requires password confirmation), the password confirmation modal's password input does not receive focus automatically anymore - so you have to manually click on it.

@taylorotwell
Copy link
Member

@xiCO2k there are also some merge conflicts because we recently added confirmation support to the 2FA screen.

@xiCO2k
Copy link
Contributor Author

xiCO2k commented Mar 16, 2022

got it will take a look on it

@taylorotwell
Copy link
Member

Marking as draft for now - just mark as ready when you want me to take a look again.

@taylorotwell taylorotwell marked this pull request as draft March 16, 2022 17:02
@xiCO2k
Copy link
Contributor Author

xiCO2k commented Mar 16, 2022

Ok should be good.

@xiCO2k xiCO2k marked this pull request as ready for review March 16, 2022 17:52
@taylorotwell
Copy link
Member

@xiCO2k did you use any specific formatter / prettier plugin to format the Vue files in terms of how HTML attributes like type and id are ordered?

@xiCO2k
Copy link
Contributor Author

xiCO2k commented Mar 16, 2022

@taylorotwell used this .eslintrc.js:

module.exports = {
    extends: [
        'eslint:recommended',
        'plugin:vue/vue3-recommended',
    ],
    ignorePatterns: ['node_modules/**/*', 'vendor/**/*', 'public/**/*'],
    parserOptions: {
        ecmaVersion: 12,
        sourceType: 'module',
    },
    plugins: [
        'vue',
    ],
    env: {
        browser: true,
        es2021: true,
        node: true,
    },
    rules: {
        indent: ['error', 4],
        quotes: ['warn', 'single'],
        semi: ['warn'],
        'no-unused-vars': ['error', { vars: 'all', args: 'after-used', ignoreRestSiblings: true }],
        'comma-dangle': ['warn', 'always-multiline'],
        'vue/html-indent': ['error', 4],
        'vue/multi-word-component-names': ['off'],
        'vue/require-default-prop': ['off'],
        'vue/max-attributes-per-line': ['error', {
            singleline: { max: 3 },
        }],
    },
    globals: {
        route: 'readonly',
        axios: 'readonly',
        defineProps: 'readonly',
        defineEmits: 'readonly',
    },
};

@taylorotwell
Copy link
Member

@xiCO2k

If I click "Enable" on two factor authentication it still doesn't seem to focus on the password box:

CleanShot 2022-03-16 at 16 21 05@2x

@xiCO2k
Copy link
Contributor Author

xiCO2k commented Mar 16, 2022

sorry fixed on other area.

@xiCO2k
Copy link
Contributor Author

xiCO2k commented Mar 16, 2022

should be good.

const codeInput = ref(null);

const toggleRecovery = async () => {
recovery.value ^= true;
Copy link
Member

@taylorotwell taylorotwell Mar 16, 2022

Choose a reason for hiding this comment

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

@xiCO2k What is ^=?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also did not know that one (was there before) but looks like it works like a toggle, if the value is 1 the ^= changes to 0 and if is 0 it changes to 1.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha

Choose a reason for hiding this comment

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

It's a bitwise XOR assignment operator, works in PHP as well.

@taylorotwell taylorotwell merged commit 6a7c9ef into laravel:2.x Mar 16, 2022
@xiCO2k xiCO2k deleted the feat/update-inertia-login-stub-to-composition-api branch March 16, 2022 22:19
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