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

Rework settings #884

Merged
merged 31 commits into from
May 13, 2024
Merged

Rework settings #884

merged 31 commits into from
May 13, 2024

Conversation

Kuuuube
Copy link
Member

@Kuuuube Kuuuube commented May 5, 2024

Summary of changes:

Welcome Page:

- Removed Scan delay (in milliseconds) (note: it has been moved to advanced in settings and we shouldn't be showing advanced settings on the welcome page)
- Removed Auto-hide search popup

- Moved "Quick Start Guide" to a separate page and added a category and button to access it from the welcome page

- Added language select option

- Bolded Recommended Permissions (Important), added warning when not enabled, success message when enabled, moved to top of the page

Settings Page:

Moved:
- Moved General category above Dictionaries category
- Moved Frequency sorting dictionary from General to Dictionaries
- Moved Result grouping mode from General to Result Display
- Moved Reading mode from Text Parsing to Appearance
- Moved Auto-hide search popup from Scanning to Popup Behavior
- Moved Hide popup on cursor exit from Scanning to Popup Behavior

Renamed:
- Popup Appearance category renamed to Appearance (note: it was already named Appearance on the sidebar, its settings affect things besides the popup so this name is better)
- Enable content scanning renamed to Enable Yomitan
- Popup to Popup Behavior

Added to advanced:
- Dictionaries:
  - Frequency sorting dictionary
    - Frequency sorting mode (note: we already detect whether a freq sorting dict is occurance based or rank based, this setting is only needed when the dict is bugged)
- Scanning:
  - Scan delay (in milliseconds)
- Text Parsing:
  - Parse sentences using Yomitan's internal parser
- Clipboard:
  - Maximum clipboard text search length
- Appearance
  - Reading mode
  - Configure custom CSS...

Removed from advanced:
- Dictionaries:
  - Frequency sorting dictionary
- Appearance:
  - Pitch accent display styles

Added warning:
- Scanning:
  - Text scan length

Added category:
- Result Display

Open to feedback and additional changes.

@Kuuuube Kuuuube requested a review from a team as a code owner May 5, 2024 18:19
@Kuuuube Kuuuube added kind/enhancement The issue or PR is a new feature or request area/ui-ux The issue or PR is related to UI/UX/Design labels May 5, 2024
Copy link

github-actions bot commented May 5, 2024

⚠️ Visual differences introduced by this PR; please validate if they are desirable.

View Playwright Report (note: open the "playwright-report" artifact)

Copy link
Collaborator

@StefanVukovic99 StefanVukovic99 left a comment

Choose a reason for hiding this comment

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

I think reordering the general section is ok, all the things that got moved to advanced also ok.

When moving things out of advanced, I would generally err on the side of keeping them there, so settings are streamlined for new users. Users following guides will necessarily customize a lot of stuff, so they will need Advanced. Specifically, i think there tends to be bias towards features that recently got attention, even though the default is ok for most new users (like the duplicate setting here; don't think the reading mode is changed often either?).

ext/settings.html Outdated Show resolved Hide resolved
ext/settings.html Outdated Show resolved Hide resolved
@Kuuuube
Copy link
Member Author

Kuuuube commented May 5, 2024

When moving things out of advanced, I would generally err on the side of keeping them there, so settings are streamlined for new users.

I think ideally we want users to not have to use advanced unless they really need to do something out of the ordinary. Some guides will for sure be doing this ofc.

The only one I've pulled out of advanced that I can see being an issue is the anki dupe checking. That one could reasonably go back in for sure.

Frequency sorting dicts almost everyone uses so we shouldn't have that in advanced. And the other 3 appearance options just seem like really basic stuff that should just be there.

ext/settings.html Outdated Show resolved Hide resolved
ext/settings.html Outdated Show resolved Hide resolved
@Kuuuube
Copy link
Member Author

Kuuuube commented May 12, 2024

Added a Result Display category

Screenshot_2024-05-12_11-09-20

@Kuuuube
Copy link
Member Author

Kuuuube commented May 12, 2024

Everything should be ready for this PR.

@Kuuuube
Copy link
Member Author

Kuuuube commented May 12, 2024

New page looks:

Welcome page Quick start guide page Settings page Settings page advanced
Screenshot_2024-05-12_13-30-53 Screenshot_2024-05-12_13-31-39 chrome-extension___ohgbcgkodmddpcggcmnanmnemelgnhcb_settings html (3) chrome-extension___ohgbcgkodmddpcggcmnanmnemelgnhcb_settings html (4)

Copy link
Collaborator

@jamesmaa jamesmaa left a comment

Choose a reason for hiding this comment

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

I think after this PR we should really prioritize making it easy to install dictionaries without leaving yomitan. The welcome page looks good and polished enough to fool people into thinking yomitan will work out of the box when in reality that's not the case

@jamesmaa jamesmaa added this pull request to the merge queue May 13, 2024
Merged via the queue into yomidevs:master with commit 0b02bf4 May 13, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui-ux The issue or PR is related to UI/UX/Design kind/enhancement The issue or PR is a new feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants