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

feat: 🍰 Post Editor Legend #4492

Merged
merged 10 commits into from
Jul 19, 2021
Merged

feat: 🍰 Post Editor Legend #4492

merged 10 commits into from
Jul 19, 2021

Conversation

Blargian
Copy link
Contributor

@Blargian Blargian commented Jun 22, 2021

🍰 Pullrequest

Added a new component called MenuLegend (adapted the content menu component) with localised content for Deutsch, English and Russian. The legend displays when the user hovers over it (desktop) or when the user clicks on the button (mobile/tablet).

image

Issues

fixes #3673

Todo

  • Add a component for the menu legend
  • Add a question-mark icon on the editor menu
  • Display it on hover rather than on click
  • Make it multilingual
  • Make it responsive
  • Fix failing tests

@Tirokk Tirokk changed the title [WIP] - 3673 post editor legend feat: [WIP] 🍰 Post Editor Legend Jun 23, 2021
@Tirokk Tirokk marked this pull request as draft June 23, 2021 13:42
package.json Outdated
Comment on lines 52 to 54
},
"dependencies": {
"jq": "^1.7.2"
Copy link
Member

@Tirokk Tirokk Jun 23, 2021

Choose a reason for hiding this comment

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

I don’t understand where all the new dependencies are coming from, because you didn’t invent anything new, as far I can see …
Here and the others below … 👇🏼

Any ideas? @Blargian @Mogge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tirokk this one @Mogge said I would need when he helped me get the dev environment setup. It's required for implementing multilanguage support or for testing related to that. 😃

Copy link
Member

Choose a reason for hiding this comment

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

Hey @Mogge , isn’t this needed for the webapp only? And not for the main folder … 🤔

Copy link
Contributor

@Mogge Mogge Jun 27, 2021

Choose a reason for hiding this comment

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

It runs over the scripts folder in the main folder. So I guess it is right there. We have to see what happens when running yarn locales.

Comment on lines 74 to 76
data() {
return {}
},
Copy link
Member

Choose a reason for hiding this comment

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

In case it is empty you can leave it away, I guess. @Blargian 👍🏼

yarn.lock Outdated
Comment on lines 1720 to 1726
bindings@^1.2.1:
version "1.5.0"
resolved "https://registry.yarnpkg.com/bindings/-/bindings-1.5.0.tgz#10353c9e945334bc0511a6d90b38fbc7c9c504df"
integrity sha512-p2q/t/mhvuOj/UeLlV6566GD/guowlr0hHxClI0W9m7MWYkL1F0hLo+0Aexs9HSPCtR1SXQ0TD3MMKrXZajbiQ==
dependencies:
file-uri-to-path "1.0.0"

Copy link
Member

@Tirokk Tirokk Jun 23, 2021

Choose a reason for hiding this comment

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

same same ☝🏼

@Tirokk
Copy link
Member

Tirokk commented Jun 23, 2021

Hey @Blargian !

This looks quite nice for a start. 🤗😍
And for a VueJS beginner … 👍🏼
So I’m looking forward to see it developing …

I have left you little hints …

return {
hover: false,
legenditems: [
{ iconname: 'bold', name: 'bold', shortcut: 'Ctrl+b' },
Copy link
Contributor

@Mogge Mogge Jun 27, 2021

Choose a reason for hiding this comment

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

Fett

hover: false,
legenditems: [
{ iconname: 'bold', name: 'bold', shortcut: 'Ctrl+b' },
{ iconname: 'italic', name: 'italic', shortcut: 'Ctrl+i' },
Copy link
Contributor

@Mogge Mogge Jun 27, 2021

Choose a reason for hiding this comment

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

Kursiv

{ iconname: 'bold', name: 'bold', shortcut: 'Ctrl+b' },
{ iconname: 'italic', name: 'italic', shortcut: 'Ctrl+i' },
{ iconname: 'underline', name: 'underline', shortcut: 'Ctrl+u' },
{ iconname: 'link', name: 'link', shortcut: '' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Unterstrichen

{ iconname: 'italic', name: 'italic', shortcut: 'Ctrl+i' },
{ iconname: 'underline', name: 'underline', shortcut: 'Ctrl+u' },
{ iconname: 'link', name: 'link', shortcut: '' },
{ iconname: 'paragraph', name: 'paragraph', shortcut: '' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Absatz

{ iconname: 'underline', name: 'underline', shortcut: 'Ctrl+u' },
{ iconname: 'link', name: 'link', shortcut: '' },
{ iconname: 'paragraph', name: 'paragraph', shortcut: '' },
{ label: 'H3', name: 'heading 3', shortcut: '### + space' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Überschrift 3

{ iconname: 'link', name: 'link', shortcut: '' },
{ iconname: 'paragraph', name: 'paragraph', shortcut: '' },
{ label: 'H3', name: 'heading 3', shortcut: '### + space' },
{ label: 'H4', name: 'heading 4', shortcut: '#### + space' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Überschrift 4

{ iconname: 'paragraph', name: 'paragraph', shortcut: '' },
{ label: 'H3', name: 'heading 3', shortcut: '### + space' },
{ label: 'H4', name: 'heading 4', shortcut: '#### + space' },
{ iconname: 'list-ul', name: 'unordered list', shortcut: '* + space' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Ungeordnete Liste

{ label: 'H3', name: 'heading 3', shortcut: '### + space' },
{ label: 'H4', name: 'heading 4', shortcut: '#### + space' },
{ iconname: 'list-ul', name: 'unordered list', shortcut: '* + space' },
{ iconname: 'list-ol', name: 'ordered list', shortcut: '1. + space' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Geordnete Liste

{ label: 'H4', name: 'heading 4', shortcut: '#### + space' },
{ iconname: 'list-ul', name: 'unordered list', shortcut: '* + space' },
{ iconname: 'list-ol', name: 'ordered list', shortcut: '1. + space' },
{ iconname: 'quote-right', name: 'quote', shortcut: '> + space' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Zitat

Copy link
Contributor Author

@Blargian Blargian Jun 27, 2021

Choose a reason for hiding this comment

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

I gave you the German translations for the legend. What happens, if you run yarn locales --fix in the webapp folder?

Thanks for the German translations! yarn locales --fix runs two scripts in scripts/translations, they both result in "jq: command not found". I tried removing jq from the main package.json file and adding the dependency just to the webapp but it gives the same message. An empty [].json file gets added to the locales.

I was able to sort the locales by running sh sort.sh --fix from within scripts/translations. Doing the same for missing-keys.sh gives an error:

missing-keys.sh: eval: line 10: syntax error near unexpected token (' missing-keys.sh: eval: line 10: diff -q <( jq -f ./../../scripts/translations/sort_filter.jq ./../../webapp/locales/en.json | jq
-c 'path(..)|[.[]|tostring]|join(".")' ) <( jq -f ./../../scripts/translations/sort_filter.jq ./../../webapp/locales/de.json | jq
-c 'path(..)|[.[]|tostring]|join(".")' )'

Copy link
Member

Choose a reason for hiding this comment

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

Could this be related? https://github.com/Ocelot-Social-Community/Ocelot-Social/blob/master/.github/workflows/test.yml#L25
I am not sure about all those translation tools & checks, but we have two scripts executed in our test workflow.
@Mogge

{ iconname: 'list-ul', name: 'unordered list', shortcut: '* + space' },
{ iconname: 'list-ol', name: 'ordered list', shortcut: '1. + space' },
{ iconname: 'quote-right', name: 'quote', shortcut: '> + space' },
{ iconname: 'minus', name: 'ruler', shortcut: '---' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Linie

Copy link
Contributor

@Mogge Mogge left a comment

Choose a reason for hiding this comment

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

I gave you the German translations for the legend. What happens, if you run yarn locales --fix in the webapp folder?

@ulfgebhardt ulfgebhardt added this to the 🏃 21/06 June milestone Jun 27, 2021
@Blargian
Copy link
Contributor Author

Blargian commented Jun 27, 2021

I pushed now the changes so that the legend displays when the user hovers over it. It is also now localised for English, Deutsch and Russian so I think the functionality is all there now. I'm getting some tests failing however - one of the backend unit tests and one of the cyprus tests.

@Blargian Blargian requested a review from Tirokk June 30, 2021 12:29
@Blargian
Copy link
Contributor Author

Hi @Tirokk, I fixed up one issue which was causing a cypress test to fail. Also removed jq as a dependency all together as it also seemed to be causing some tests to fail.

@Blargian Blargian marked this pull request as ready for review June 30, 2021 12:31
@Tirokk Tirokk requested review from ulfgebhardt and Mogge June 30, 2021 13:04
@Tirokk Tirokk changed the title feat: [WIP] 🍰 Post Editor Legend feat: 🍰 Post Editor Legend Jun 30, 2021
- Remove the split line under the last list item.
- Enter spaces.
- Partial swapping of upper and lower case letters.
Copy link
Member

@Tirokk Tirokk left a comment

Choose a reason for hiding this comment

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

Hey @Blargian ,

I’m back from holidays 🏝 and have reviewed your PR.
Thanks for your patience and sorry for the delay! 🙏🏼

You did a really great job. Very cool 😍🚀💫💫

I have taken the liberty of changing a few small things that I noticed. Nothing serious. 👍🏼
Hope that’s okay for you?

Please merge, if the tests are successful! 🥰

PS: Please let me know if you like to care for those things for your own in the future.

@Tirokk
Copy link
Member

Tirokk commented Jul 19, 2021

Hey dear @Blargian ,
then I merge your PR into our code base. 😍

@Tirokk Tirokk merged commit e77848a into master Jul 19, 2021
@Tirokk Tirokk deleted the 3673_post_editor_legend branch July 19, 2021 08:52
@Blargian
Copy link
Contributor Author

Hi @Tirokk! Sorry for not replying. I saw your message and then got a bit busy and forgot to respond. Of course I don't mind for you to make some changes. 🤗 Thank you

@Tirokk
Copy link
Member

Tirokk commented Jul 20, 2021

There is an console warning I and @Blargian have overseen: #3673 (comment)

I try to fix this …

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 [Feature] Legend of post editor toolbar buttons
4 participants