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

Move all login and account creation page labels to be above inputs #29432

Merged
merged 24 commits into from
Mar 6, 2024

Conversation

rafh
Copy link
Contributor

@rafh rafh commented Feb 26, 2024

There are a few inconsistencies within Gitea and this PR addresses one of them. This PR updates the sign-in page layout, including the register and openID tabs, to match the layout of the settings pages (/user/settings) for more consistency.

This PR updates the following routes:
/user/login
/user/sign_up
/user/login/openid
/user/forgot_password
/user/link_account
/user/recover_account

Before
Screenshot 2024-02-05 at 8 27 24 AM

After
Screenshot 2024-02-05 at 8 26 39 AM

This PR addresses a revert of the original PR due to this comment.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 26, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 26, 2024
@KN4CK3R
Copy link
Member

KN4CK3R commented Feb 26, 2024

Could you have a look at the two factor page too? Last time I checked the content was not centered.
grafik

@silverwind
Copy link
Member

Looks cleaner with that layout, I like it.

@zokkis
Copy link
Contributor

zokkis commented Feb 26, 2024

Maybe remove the 2. title?
Because now there is the tab with "sign in" and directly under the tab is the title with "sign in"

{{.Captcha.CreateHTML}}
</div>
<div class="required inline field {{if .Err_Captcha}}error{{end}}">
<label for="captcha">{{ctx.Locale.Tr "captcha"}}</label>
<input id="captcha" name="captcha" value="{{.captcha}}" autocomplete="off">
<input id="captcha" class="gt-w-full" name="captcha" value="{{.captcha}}" autocomplete="off">
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized that it is playing a CSS style overriding game ....

For example: by Fomantic UI: the inline field above means that the label is the same row as the input

https://fomantic-ui.com/collections/form.html#inline-field

So, by default, without the inline, it should be the UI you need.

I do not think it is right to use "inline" outside, and change the "inline" behavior inside.

There are already many UI bugs caused by such unclear overridings.

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 understand this perspective. I removed the inline class to allow the default behavior of the div to take over.

On the topic of helper classes, we recently implemented tailwinds. We should update our contribution docs to reflect the usage of these classes? If tailwinds is the path forward, I'll open a PR to update these docs.

12. Gitea's tailwind-style CSS classes use `gt-` prefix (`gt-relative`), while Gitea's own private framework-level CSS classes use `g-` prefix (`g-modal-confirm`).

Copy link
Contributor

@wxiaoguang wxiaoguang Mar 2, 2024

Choose a reason for hiding this comment

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

I think it should be updated accordingly. At the moment the plan is to remove most Gitea's own helpers (gt-), use Tailwind (tw-) as much as possible.

@lunny
Copy link
Member

lunny commented Feb 27, 2024

I personally like the previous style. It's more beautiful to not align every line.

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

Put a "request change" by #29432 (comment)

Personally I think it should clearly define the HTML layout and styles, instead of overriding the styles to a different purpose.

If most people agree to override, feel free to dismiss this request change.

Thank you.

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 27, 2024
@rafh rafh requested a review from wxiaoguang March 1, 2024 18:13
@wxiaoguang wxiaoguang dismissed their stale review March 2, 2024 03:03

dismiss old change request

@GiteaBot GiteaBot removed the lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged label Mar 2, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 2, 2024
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

Thank you very much. Without the "inline" patches, the new approach looks better to me.

  • It seems that the ".user.activate" form is still using "inline", should it be updated too?
  • Why sometimes there is "md:tw-max-w-2xl tw-m-auto" but sometimes there isn't? Could there be some general (unified) framework approaches?

@rafh
Copy link
Contributor Author

rafh commented Mar 5, 2024

  • md:tw-max-w-2xl

I updated the activate template to no longer be inline.

This line of CSS is the reason for the use of md:tw-max-w-2xl tw-m-auto. To create a unified approach, I removed the CSS and updated the templates that used it. The PR grew, but things are now more consistent.

@wxiaoguang
Copy link
Contributor

Thank you, it looks better. Actually, I am not a fan of fine-tuning everything, that's why I proposed to use some clearly defined CSS classes for these forms. But the current approach is not unacceptable ....

The last question: why user.activate form is left there? I think it also belongs to the "user sign in/out" pages.

@rafh
Copy link
Contributor Author

rafh commented Mar 5, 2024

Thank you, it looks better. Actually, I am not a fan of fine-tuning everything, that's why I proposed to use some clearly defined CSS classes for these forms. But the current approach is not unacceptable ....

The last question: why user.activate form is left there? I think it also belongs to the "user sign in/out" pages.

It was still being used by the activate_prompt.tmpl and prohibit_login.tmpl. I removed the CSS and updated the tmpl to align with the previous changes.

@rafh rafh requested a review from wxiaoguang March 5, 2024 20:52
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 6, 2024
@silverwind
Copy link
Member

silverwind commented Mar 6, 2024

Sidenote: Please don't raise PR's from the default branch main for reasons mentioned here and because it's a pain for reviewers to work with that branch as they need to rename their local branch.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 6, 2024
@silverwind silverwind enabled auto-merge (squash) March 6, 2024 14:03
@silverwind silverwind merged commit c996e35 into go-gitea:main Mar 6, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Mar 6, 2024
silverwind added a commit to silverwind/gitea that referenced this pull request Mar 6, 2024
* origin/main: (29 commits)
  Tweak actions color and borders (go-gitea#29640)
  Add download URL for executable files (go-gitea#28260)
  Move all login and account creation page labels to be above inputs (go-gitea#29432)
  Avoid issue info panic (go-gitea#29625)
  Cache repository default branch commit status to reduce query on commit status table (go-gitea#29444)
  Avoid unexpected panic in graceful manager (go-gitea#29629)
  Add a link for the recently pushed branch notification (go-gitea#29627)
  Fix wrong header of org project view page (go-gitea#29626)
  Detect broken git hooks (go-gitea#29494)
  Sync branches to DB immediately when handle git hook calling (go-gitea#29493)
  Fix wrong line number in code search result (go-gitea#29260)
  Make wiki default branch name changable (go-gitea#29603)
  A small refactor for agit implementation (go-gitea#29614)
  Update Twitter Logo (go-gitea#29621)
  Fix 500 error when adding PR comment (go-gitea#29622)
  Run editorconfig-checker on `locale_en-US.ini` (go-gitea#29608)
  bump protobuf module (go-gitea#29617)
  Add ac claim for old docker/build-push-action@v3 / current buildx gha cache (go-gitea#29584)
  Skip email domain check when admins edit user emails (go-gitea#29609)
  Improve natural sort (go-gitea#29611)
  ...
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 7, 2024
* giteaofficial/main:
  Use strict protocol check when redirect (go-gitea#29642)
  Update various logos and unify their filenames (go-gitea#29637)
  Tweak actions color and borders (go-gitea#29640)
  Add download URL for executable files (go-gitea#28260)
  Move all login and account creation page labels to be above inputs (go-gitea#29432)
  Avoid issue info panic (go-gitea#29625)
  Cache repository default branch commit status to reduce query on commit status table (go-gitea#29444)
  Avoid unexpected panic in graceful manager (go-gitea#29629)
  Add a link for the recently pushed branch notification (go-gitea#29627)
  Fix wrong header of org project view page (go-gitea#29626)
  Detect broken git hooks (go-gitea#29494)
  Sync branches to DB immediately when handle git hook calling (go-gitea#29493)
  Fix wrong line number in code search result (go-gitea#29260)
  Make wiki default branch name changable (go-gitea#29603)
  A small refactor for agit implementation (go-gitea#29614)
  Update Twitter Logo (go-gitea#29621)
  Fix 500 error when adding PR comment (go-gitea#29622)
@lunny lunny modified the milestones: 1.23.0, 1.22.0 Mar 8, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants