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

refactor: Converting span to kbd tag #2774

Merged
merged 3 commits into from Jan 14, 2021
Merged

refactor: Converting span to kbd tag #2774

merged 3 commits into from Jan 14, 2021

Conversation

Rafi993
Copy link
Contributor

@Rafi993 Rafi993 commented Jan 13, 2021 •
edited

What type of PR is this?

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

What does it do?

  1. kbd tag is used to keyboard shortcuts
  2. converting span to kbd tag to be more semtantic

Related issue

NA

QA Instructions, Screenshots, Recordings

  1. checkout the branch
  2. view keyboard shortcut modal it should use kbd tag instead of span nothing else should have changed

UI accessibility concerns?

  • With the way keybard shortcut is now rendered it uses single tag for rendering a combination. For example ctrl + c gets rendered inside a single tag it would be better if ctrl and c gets rendered as a separate tag
  • This is more of a preference than an actual accessibility concerns

Tests?

  • yes
  • no, reason?
  • I need help with writing tests
    There were no existing snapshot covering this I was not sure if I should add one

@Rafi993
Converting span to kbd tag
d4a161f 
1. kbd tag is used to keyboard shortcuts
2. converting span to kbd tag to be more semtantic
@vercel
Copy link

vercel bot commented Jan 13, 2021 •
edited

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/excalidraw/excalidraw/mxck5rdl0
✅ Preview: https://excalidraw-git-fork-rafi993-converting-span-to-kdb.excalidraw.now.sh

@dwelle
Copy link
Member

dwelle commented Jan 13, 2021

I'm fine with <kbd>, but right now it changes our styling, so we'd have to reset:

image

After:

image

You can add a font-family: inherit; to the .ShorcutsDialog-key class.

@dwelle
Copy link
Member

dwelle commented Jan 13, 2021

As for:

With the way keybard shortcut is now rendered it uses single tag for rendering a combination. For example ctrl + c gets rendered inside a single tag it would be better if ctrl and c gets rendered as a separate tag

IMO it would make it look like alternative shortcuts instead:

image

@lipis
Copy link
Member

lipis commented Jan 13, 2021

If we changing it here.. we should change in the context menu too..

@Rafi993
Copy link
Contributor Author

Rafi993 commented Jan 14, 2021

font

I have added font-family: inherit this is how it looks now.

@Rafi993
Copy link
Contributor Author

Rafi993 commented Jan 14, 2021

If we changing it here.. we should change in the context menu too..

I have converted div in context menu to kbd key too now

context_menu

@lipis lipis changed the title Converting span to kbd tag refactor: Converting span to kbd tag Jan 14, 2021
@lipis lipis merged commit 511eb62 into excalidraw:master Jan 14, 2021
@Rafi993
Copy link
Contributor Author

Rafi993 commented Jan 14, 2021

Thank you @lipis

lipis added a commit that referenced this pull request Jan 15, 2021
@lipis
Merge branch 'master' of github.com:excalidraw/excalidraw into cleanipp
1eb8920 
* 'master' of github.com:excalidraw/excalidraw: (37 commits)
  feat: Add toast (#2772)
  docs: Update readme with documentation (#2788)
  fix: allow text-selecting in dialogs & reset cursor (#2783)
  chore: Update translations from Crowdin (#2742)
  fix: broken Individuals link (#2782)
  refactor: Converting span to kbd tag (#2774)
  fix: don't render due to zoom after unmount (#2779)
  fix: Track the chart type correctly (#2773)
  chore(deps-dev): bump terser-webpack-plugin in /src/packages/excalidraw (#2750)
  chore(deps-dev): bump webpack in /src/packages/utils (#2768)
  fix: delay version logging & prevent duplicates (#2770)
  chore(deps-dev): bump webpack in /src/packages/excalidraw (#2769)
  chore(deps-dev): bump ts-loader in /src/packages/excalidraw (#2749)
  chore(deps-dev): bump ts-loader in /src/packages/utils (#2753)
  chore(deps): bump nanoid from 2.1.11 to 3.1.20 (#2581)
  feat: Track current version (#2731)
  chore(actions): Use cancel workflow action (#2763)
  chore(deps): bump @testing-library/react from 11.2.2 to 11.2.3 (#2755)
  chore(deps-dev): bump firebase-tools from 9.1.0 to 9.1.2 (#2761)
  chore(deps-dev): bump eslint-plugin-prettier from 3.3.0 to 3.3.1 (#2754)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lipis lipis lipis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Rafi993 @dwelle @lipis