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

imprv: Fix flow from subnav #8822

Closed

Conversation

TatsuyaIse
Copy link
Contributor

@TatsuyaIse TatsuyaIse commented May 20, 2024

task

https://redmine.weseek.co.jp/issues/146145

window image

  1. Registration disabled open wiki (enable guest user access) case
  • image
  1. Registration enabled open wiki case
  • image
  1. Registration disabled shared page case
  • image
  1. Registration enabled shared page case
  • image

Copy link
Member

Choose a reason for hiding this comment

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

/pages/[[...path]].page.tsx と /pages/share/[[...path]].page.tsx で利用する程度でしたら、commons ではなくそれらの Next Page に直接取得処理を書くで良いかもしれないです。

props.currentUser など大半の Next Page で利用する値の取得を commons に書くイメージです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

common から取り除き、それぞれのページコンポーネントで個別にサーバサイドから取得する実装を加えました。

- move isRegistrationEnabled from common to specific files
Copy link

reg-suit bot commented May 21, 2024

reg-suit detected visual differences.

Check this report, and review them.

🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴

⚫⚫⚫⚫⚫⚫⚫⚫⚫⚫⚫⚫⚫⚫
🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵

What do the circles mean? The number of circles represent the number of changed images.
🔴 : Changed items, ⚪ : New items, ⚫ : Deleted items, and 🔵 Passed items

How can I change the check status? If reviewers approve this PR, the reg context status will be green automatically.

@@ -45,6 +47,7 @@ type Props = CommonProps & {
isSearchServiceReachable: boolean,
isSearchScopeChildrenAsDefault: boolean,
isEnabledMarp: boolean,
isRegistrationEnabled: boolean,
Copy link
Member

Choose a reason for hiding this comment

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

isLocalAccountRegistrationEnabled に変更する

<Link href="/login#register" className="btn me-2" prefetch={false}>
<span className="material-symbols-outlined me-1">person_add</span>{t('Sign up')}
</Link>
)}
Copy link
Member

Choose a reason for hiding this comment

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

ストーリーの期待される動作通りではないが、それがベストではない気もする。
再考してほしい。

グレーアウト案

ストーリーの説明ではグレーアウトさせるのが期待される動作となっている
disabled にするのであれば tooltip も一緒に実装すべき

ボタンを表示しない案

OIDC や SAML で初回ログインすれば新規作成できる場合、「ログイン」ボタンをクリックする気になるだろうか?

表示はするが href をログイン画面にする案

ボタンは表示しつつ、href を /login にする?

ログインボタンのラベルを「新規作成 or ログイン」にする案

ただし local account が有効だろうが無効だろうが、OIDC / SAML でも新規アカウント作成できる点はかわらない
その条件で変えてしまっていいだろうか?

Copy link

changeset-bot bot commented May 29, 2024

⚠️ No Changeset found

Latest commit: 26aeba4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@yuki-takei yuki-takei closed this Jul 1, 2024
@yuki-takei yuki-takei deleted the imprv/144159-146145-fix-flow-from-login-to-register branch July 1, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

[7.0.1]自己登録できない設定の場合でも画面右上に「新規登録」が表示される
3 participants