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.

Sign up for GitHub

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

Jump to bottom

Update login note #3424

Merged
merged 3 commits into from
Mar 23, 2022
Merged

Update login note #3424

merged 3 commits into from
Mar 23, 2022

Conversation

khanhtc1202
Copy link
Member

@khanhtc1202 khanhtc1202 commented Mar 22, 2022 •
edited
Loading

What this PR does / why we need it:

It would look like this

Screen Shot 2022-03-23 at 9 35 27

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 35.11%. This pull request does not change code coverage.

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for javascript is 78.33%. This pull request increases coverage by 0.08%.

File Base Head Diff
src/modules/commands/index.ts 90.62% 93.75% +3.12%
src/modules/project/index.ts 56.60% 60.38% +3.77%

knanao
knanao reviewed Mar 23, 2022
View reviewed changes
web/src/components/login-page/index.tsx Outdated
@@ -101,7 +101,7 @@ export const LoginPage: FC = memo(function LoginPage() {
/>
{isPlayEnvironment && (
<div className={classes.note}>
Try with <strong>play</strong> name if you want to join the
Try project name <strong>play</strong> if you want to join the
Copy link
Member

Choose a reason for hiding this comment

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

nits

Suggested change
Try project name <strong>play</strong> if you want to join the
Try to input <strong>play</strong> as project name if you want to join the

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice idea, but this text is a bit too long to be shown in a single line 🙈

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I got it.
Does it not also work if you set testAlign: "left"?

Copy link
Member Author

Choose a reason for hiding this comment

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

align right
Screen Shot 2022-03-23 at 9 32 10

align left
Screen Shot 2022-03-23 at 9 32 53

🙈

Copy link
Member

Choose a reason for hiding this comment

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

😇
allright, let's keep it that way.

knanao
knanao reviewed Mar 23, 2022
View reviewed changes
Copy link
Member

@knanao knanao left a comment

Choose a reason for hiding this comment

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

Is better off textAlign: "left" ?
https://github.com/pipe-cd/pipecd/pull/3424/files#diff-859b4e56fe1e23be4f03551006e60f9de7a224b2d91542372eefb519fb58cdbbR43

@knanao
Copy link
Member

knanao commented Mar 23, 2022

/lgtm

nghialv
nghialv reviewed Mar 23, 2022
View reviewed changes
web/src/components/login-page/index.tsx Outdated
@@ -101,7 +101,7 @@ export const LoginPage: FC = memo(function LoginPage() {
/>
{isPlayEnvironment && (
<div className={classes.note}>
Try with <strong>play</strong> name if you want to join the
Try project name <strong>play</strong> if you want to join the
Copy link
Member

Choose a reason for hiding this comment

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

or just 😄

Enter <strong>play</strong> if you want to join the playground environment

Copy link
Member Author

Choose a reason for hiding this comment

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

Enter is a bit odd to me, so lets me keep it as is 😄

Copy link
Member Author

@khanhtc1202 khanhtc1202 Mar 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

But, maybe Input <strong>play</strong> if you want to join the playground environment is good. What do you think? @nghialv @knanao

Screen Shot 2022-03-23 at 9 53 35

Copy link
Member

Choose a reason for hiding this comment

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

Looks good!

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed by 2e02521 🙌

@pipecd-bot pipecd-bot removed the lgtm label Mar 23, 2022
@knanao
Copy link
Member

knanao commented Mar 23, 2022

/lgtm

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 35.11%. This pull request does not change code coverage.

@nghialv
Copy link
Member

nghialv commented Mar 23, 2022

Nice.

/approve

@pipecd-bot
Copy link
Collaborator

APPROVE

This pull request is APPROVED by nghialv.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for javascript is 78.33%. This pull request increases coverage by 0.08%.

File Base Head Diff
src/modules/commands/index.ts 90.62% 93.75% +3.12%
src/modules/project/index.ts 56.60% 60.38% +3.77%

@pipecd-bot pipecd-bot merged commit 816631d into master Mar 23, 2022
@pipecd-bot pipecd-bot deleted the update-note branch March 23, 2022 03:12
@pipecd-bot pipecd-bot mentioned this pull request Mar 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knanao knanao knanao left review comments

@nghialv nghialv nghialv left review comments

Assignees
No one assigned
Labels
approved area/web lgtm release-note-none size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@khanhtc1202 @pipecd-bot @knanao @nghialv