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

vueify login page #4634

Merged
merged 13 commits into from
Jul 12, 2018
Merged

vueify login page #4634

merged 13 commits into from
Jul 12, 2018

Conversation

OmgImAlexis
Copy link
Collaborator

We have to expose the api-root to non-authed users. I can't see this being an issue since they'll need to know this once we move to using the API for authentication anyways.

Closes #4624

@OmgImAlexis OmgImAlexis added this to the 0.2.7 milestone Jul 8, 2018
@OmgImAlexis OmgImAlexis requested a review from sharkykh July 8, 2018 00:46
@sharkykh
Copy link
Contributor

sharkykh commented Jul 8, 2018

I'm okay with exposing the api-root.
We should instead expose the web-root, though.
Then update all the base paths to stop doing the .replace, and only add /api/v2 to the base path for the api axios object.

name: 'login',
meta: {
title: 'Login',
header: 'Medusa'
Copy link
Contributor

Choose a reason for hiding this comment

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

No header for the login page. It has its own header.
It's not currently an issue, because it doesn't use the VueRouter template (index.mako),
but it will be when it does.

@@ -49,7 +49,7 @@
<link rel="stylesheet" type="text/css" href="css/lib/vue-snotify-material.css?${sbPID}"/>
<%block name="css" />
</head>
<body ${('data-controller="' + controller + '" data-action="' + action + '" api-key="' + app.API_KEY +'" api-root="' + app.WEB_ROOT + '/api/v2/"', '')[title == 'Login']}>
<body ${('data-controller="' + controller + '" data-action="' + action + '" api-key="' + app.API_KEY + '"', '')[title == 'Login']} api-root="${app.WEB_ROOT}/api/v2/">
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's expose web-root="${app.WEB_ROOT}" instead.
(see non-review comment above)

<div class="login">
<form action="" method="post">
<h1>Medusa</h1>
<div class="ctrlHolder"><input class="inlay" name="username" type="text" placeholder="Username" autocomplete="off" autocomplete="no" /></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

  static/js/templates/login.vue:5:128
  ✖  5:128  Duplicate attribute autocomplete.    vue/no-duplicate-attributes
  ✖  5:128  Parsing error: duplicate-attribute.  vue/no-parsing-error

off is the correct value.

@codecov

This comment has been minimized.

@OmgImAlexis OmgImAlexis dismissed sharkykh’s stale review July 11, 2018 23:56

Should be good to go.

@sharkykh

This comment has been minimized.

@sharkykh
Copy link
Contributor

sharkykh commented Jul 12, 2018

Latest commit (cherry picked from #4656) should fix that issue.

Copy link
Contributor

@sharkykh sharkykh left a comment

Choose a reason for hiding this comment

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

Tested and working.

@OmgImAlexis OmgImAlexis merged commit 6e2867c into develop Jul 12, 2018
@OmgImAlexis OmgImAlexis deleted the feature/vueify-login branch July 12, 2018 23:32
@OmgImAlexis OmgImAlexis mentioned this pull request Jul 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants