-
Notifications
You must be signed in to change notification settings - Fork 274
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
[#545] Toggle button to hide/show romanization added #551
base: master
Are you sure you want to change the base?
Conversation
…and changes value of button correctly
Hi, @anjvyas thank you for the PR. As the repo is currently on vacation mode, new additions won't come up soon, and there will be some delay in the progress. We will catch up soon when we are able to maintain. Have a good day! |
I hope you have a great day as well. This is an awesome extension and we really enjoyed working on it :) @jayehernandez @Kvaibhav01 thank you for the opportunity! If you find that there are issues with what we wrote, we can definitely work on it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution, @anjvyas and @bhavinip! I just have some suggestions as to how we can improve this:
It seems like the user will have to press on the "Hide Romanization" button every time the extension is opened. This may be a hassle to users as the option they want won't be saved. It may be better to save it to Chrome storage along with the dropdown of languages. (Sample screenshot, we'll have to add in a checkbox here)
To go about this we can introduce an options
storage in Chrome, which we will get and set through store/index.js
, similar to how selectedLanguages
is stored. This can also be used in the future if we'll add in more settings. Here's what I think we'll need to do, but if you have any suggestions do bring it up!
- Add in the romanization checkbox inside the
Options.vue
component. We'll have to reference its value insidedata
with a default oftrue
:
data() {
return {
options: { showRomanization: true },
};
},
- On save, call an action from the store that will save the options to chrome storage, and commit the change in options to the store.
- Inside
Word.vue
, we'll have to usemapState
to get the options we set, and this will be the basis on if we'll be seeing the word's romanization or not. - This should also work for when the user opens up another tab, so on
mounted
inApp.vue
, we'll have to add another action in the store that will get the options from Chrome storage and store it. Since we added the step 3, it should work as well when the user opens a new tab.
To debug, you can download the Storage Area Explorer extension so you can see what is being stored in Chrome.
Feel free to let me know if you'll be needing any help -- this is already such a great start to solving the issue in itself, with the added tests as well. :)
@bhavinip and I added a button to show/hide romanization as suggested in #545!
We extended it beyond Chinese and made it so that the button shows up in all languages that have romanization information available. We also made it so that it does not show up in languages that do not have the information available. In the default state the romanization is displayed and the button (positioned above it) says 'Hide romanization.' When the button is pressed it hides the romanization and then says 'Show romanization.' This part was implemented in Word.vue.
We also wanted to keep the button consistent with letra's existing aesthetic so when you hover over it, it turns yellow like the other buttons and the cursor also changes to a pointer! (made changes in app.scss for this).
Finally, we also added some tests in the file Word.test.js that check to make sure that: