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

Fine tune markdown editor toolbar #24046

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Apr 11, 2023

  1. Remove unnecessary btn-link muted classes
    • Link is link, button is button, I can't see a real requirement to make a button like a link.
    • If anyone prefers it, please help to show some real examples from modern frameworks / websites, how and why they do so.
    • No need to duplicate a lot of class names on similar elements
    • Declare styles clearly, for example, markdown-toolbar itself should have display: flex, but not use gt-df to overwrite the display: block.
  2. Remove unnecessary role attribute
  3. Indent markdown-switch-easymde (before it doesn't have a proper indent)

Screenshot:

image

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Apr 11, 2023
@yardenshoham yardenshoham added this to the 1.20.0 milestone Apr 11, 2023
@yardenshoham yardenshoham added topic/ui Change the appearance of the Gitea UI outdated/theme/markdown labels Apr 11, 2023
@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 Apr 11, 2023
@lunny lunny merged commit 704f3aa into go-gitea:main Apr 11, 2023
@wxiaoguang wxiaoguang deleted the fine-tune-editor-toolbar branch April 11, 2023 08:36
@silverwind
Copy link
Member

silverwind commented Apr 11, 2023

Just to note, the role=button is added purely for a11y. I do find the addition via JS sub-optimal when we can do the same via HTML. If the mentioned bug is fixed and the element self-sets the role, it'll be fine as well.

@wxiaoguang
Copy link
Contributor Author

If upstream fixes the bug, or the JS code causes any problem, I will do following fix.

@silverwind
Copy link
Member

silverwind commented Apr 11, 2023

I think we should generally prefer setting attributes via HTML. It works even when JS is disabled and is instantly correct on page load.

@wxiaoguang
Copy link
Contributor Author

  1. The JS approach is the same as upstream, it doesn't cause any problem , no performance problem.
  2. It's easier to maintain, if upstream fixes the bug, just delete these 2 line JS code
  3. It makes the template code clearer, no more duplicate code
  4. Does "works even when JS is disabled and is instantly correct on page load" make sense? Without JS, does any button work?

@silverwind
Copy link
Member

Keep in mind there is still a minority of users who run Gitea with JS disabled. We don't really support them, but I wouldn't make their life harder knowingly by moving stuff to JS.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Apr 11, 2023

If we want to make their life easier, for example, the relative-time component should show user-friendly date/time as its text content (at least, yyyy-MM-dd HH:mm:ss +TZ), but not use UTC ISO 8601/RFC3339 as its text content.

@silverwind
Copy link
Member

silverwind commented Apr 11, 2023

I suppose custom elements don't work at all with JS disabled and I don't think it's worth the effort trying to render some placeholder for such cases.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Apr 11, 2023

So you see, I do not think " wouldn't make their life harder knowingly by moving stuff to JS." makes sense either.

ps: "I suppose custom elements don't work at all" , actually , the text content just works, that's the default content without JS for end users , if you want to support them.

zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 12, 2023
* upstream/main:
  Add popup to hashed comments/pull requests/issues in file editing/adding preview tab (go-gitea#24040)
  Use reactive store to share data between components (go-gitea#23996)
  [skip ci] Updated translations via Crowdin
  Fix accidental overwriting of LDAP team memberships (go-gitea#24050)
  Add cardtype to org/user level project on creation, edit and view (go-gitea#24043)
  Fix branch protection priority (go-gitea#24045)
  Update documentation to explain which projects allow Gitea to host static pages (go-gitea#23993)
  Fix date  display bug (go-gitea#24047)
  Fine tune markdown editor toolbar (go-gitea#24046)
  Add tooltips for MD editor buttons and add `muted` class for buttons (go-gitea#23896)
  Prefer native parser for SSH public key parsing (go-gitea#23798)
@wxiaoguang wxiaoguang added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label May 2, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jul 31, 2023
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. outdated/theme/markdown skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants