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

Toggle space indicators #256

Closed
amyjko opened this issue Nov 1, 2023 · 15 comments
Closed

Toggle space indicators #256

amyjko opened this issue Nov 1, 2023 · 15 comments
Assignees
Labels
buildable When an enhancement has a design ready to be built
Milestone

Comments

@amyjko
Copy link
Collaborator

amyjko commented Nov 1, 2023

What's the problem?

The editor always shows spaces and tabs as indicators of where spacing is. One creator became confused by these, since the space indicator was the same as the product indicator, but a different color. They requested the ability to turn off the space indicators.

What's the design idea?

Just have an editor toggle that turns off spacing indicators. This could be synced as a setting in the cloud, or local to an editor, or local to a project. I think the best design is probably an account feature, since its a work preference.

Who benefits?

Any signed user editing code.

Design specification Draft

Here is the second draft of the design specification, please give me feedback and let me know if you have any questions! Thanks!

Functionality

  • Implement a simple toggle in the creator's settings. This toggle should enable users to turn on or off the spacing indicators according to their preference.

  • Ensure that the toggle is clearly labeled (e.g., "Show Space Indicators") and accessible, considering creators with different abilities, including those who rely on screen readers.

How the Setting is stored

  • When a creator modifies this setting, it should be automatically synced to their account in the cloud. This ensures that their preferences are maintained across different devices and sessions.

  • In addition to cloud syncing, the setting should also be stored locally. This is beneficial in scenarios where the user might be working offline or in a limited connectivity environment.

  • Persistence Across Sessions:

    • Auto-Sync on Login: Ensure that when a user logs out and then logs back in, the settings should keep the same.

    • Local Fallback: In case of connectivity issues, the editor should be able to fall back to the locally stored settings to ensure continuity in the user experience.

Design Considerations

  • The toggle should be easily accessible but not intrusive. It can be placed in the settings menu where creators can quickly access it without disrupting their workflow.

  • Provide immediate visual feedback when the setting is toggled. If a user disables the space indicators, the change should reflect instantly in the editor.

Design Drafts

Figma design draft
image

@amyjko amyjko added enhancement needs design When an enhancement is not yet designed labels Nov 1, 2023
@amyjko amyjko added this to the 1.0 milestone Nov 1, 2023
@leahjia leahjia self-assigned this Nov 13, 2023
@leahjia
Copy link

leahjia commented Nov 13, 2023

I attempted to add a space indicator mode in en-US.json since that is where I saw all other settings being listed, but it shows a warning saying it's not allowed.

Some notes & suggestions from @amyjko:

en-US.json is the English locale file; it contains strings that are used to localize the interface, not settings.

To make a space toggling setting, there are a number of design considerations:

  • Should it be something that's persistent between sessions?

  • If persisted, should it be editor, project, device, or creator-specific?

  • Should it be an on/off setting, or also allow for customization of the symbols used to indicate space? (And why is this customization necessary)?

  • If off, how can the creator distinguish between explicit spaces in a source file, and pseduo spaces inserted by the editor to preserve layout?

Those are all key design questions to answer before you start on an implementation, because the implementation would change depending on their answers. Design first, then build.

@amyjko
Copy link
Collaborator Author

amyjko commented Nov 13, 2023

Clarification: settings are not listed in en-US.json, or locale files. That's only for localizing strings in the interface; you likely are referring to the settings key in the locale file, which just contains the strings used in the settings dialog.

After you have an approved design, likely places to implement this, depending on the design, are SettingsDatabase, Space.svelte, Spaces.ts, and CaretView, all of which are involved in rendering spaces.

@leahjia
Copy link

leahjia commented Nov 13, 2023

Yes I was referring to the settings. I wanted to see if I could have these space replacement calls depend on a toggle somewhere (settings in this case).

After you have an approved design, likely places to implement this, depending on the design, are SettingsDatabase, Space.svelte, Spaces.ts, and CaretView, all of which are involved in rendering spaces.

Should I pull in the design team first?

@amyjko
Copy link
Collaborator Author

amyjko commented Nov 23, 2023

Eek, looks like GitHub didn't give me a notification about this comment. I just noticed.

Yes, this issue has the needs design tag, so it needs design before it can be built. Implementation comes after design. Per the design process on the wiki, this won't be ready to build until the issue has an approved design specified in the issue body above.

@momoko0719 momoko0719 self-assigned this Nov 28, 2023
@momoko0719
Copy link

Hi! I've updated the issue section with the first draft of the design specification. Please provide me with some feedback, and feel free to ask me questions if any part is unclear. Thanks!

@amyjko
Copy link
Collaborator Author

amyjko commented Nov 29, 2023

Overall, this design proposal is clear and complete. The only unresolved parts are 1) whether you're proposing to allow for space indicator customization, and 2) whether you're proposing to provide indicators for the line the text cursor is on. Design proposals are complete when they have no ambiguities left, so these have to be decided before this is ready to build.

If you do offer customization, one engineering constraint to know of is that not all symbols are equally simple to support. The font may not support the symbol and some symbols are not fixed width. So you'd likely need to limit it to a specific set of symbols. That said, I'm not sure customization is particularly valuable. I'd want to hear more about why you think it's needed, who it would benefit, and who it would harm.

@momoko0719
Copy link

Overall, this design proposal is clear and complete. The only unresolved parts are 1) whether you're proposing to allow for space indicator customization, and 2) whether you're proposing to provide indicators for the line the text cursor is on. Design proposals are complete when they have no ambiguities left, so these have to be decided before this is ready to build.

If you do offer customization, one engineering constraint to know of is that not all symbols are equally simple to support. The font may not support the symbol and some symbols are not fixed width. So you'd likely need to limit it to a specific set of symbols. That said, I'm not sure customization is particularly valuable. I'd want to hear more about why you think it's needed, who it would benefit, and who it would harm.

Thank you for your insightful feedback on the design proposal. I appreciate your comments on the clarity and completeness of the proposal. Regarding your concerns:

After reflecting on your feedback and considering the practical aspects, I agree that allowing for space indicator customization might introduce unnecessary complexity. The engineering constraints you mentioned, such as font support limitations and the varying widths of symbols, present challenges. Moreover, choosing appropriate symbols for space indicators is not a straightforward task. Given these factors, I now believe that customization may not be essential for the project's success.

As for providing indicators for the line where the text cursor is located, I propose we do not include this feature in the current scope. Keeping the design simpler at this stage will help us focus on the core functionality and ensure a more streamlined user experience. We can always consider adding this feature in future iterations based on user feedback and demand.

I've updated the design specification and I'm open to further discussion on this and look forward to your feedback, thanks!

@amyjko amyjko added buildable When an enhancement has a design ready to be built and removed needs design When an enhancement is not yet designed labels Nov 30, 2023
@amyjko
Copy link
Collaborator Author

amyjko commented Nov 30, 2023

Okay, I agree. Design approved! I've marked this buildable.

Will one or both of you be acting as the developers for this? If not, I encourage you to recruit someone.

@leahjia
Copy link

leahjia commented Nov 30, 2023

Thank you @momoko0719 @amyjko. Happy to take on this one, and I'll ask if anyone wants to collaborate.

@amyjko
Copy link
Collaborator Author

amyjko commented Dec 14, 2023

Hi @momoko0719 and @leahjia!

It's the end of the quarter, so please post an update on this issue. Things to consider:

  • Should you still be the assignee? (If you're planning on working on it, yes! Submit a comment explaining your plans. If not, then unassign yourself, so others are eligible to grab it).
  • Do you have any work products that are hosted outside of GitHub? Embed them in this issue, so others can build off of them.
  • Are there any branches associated with this issue? Please link to them in the issue.

If there's nothing to change above, then at least post a comment indicating that you've seen this. Thanks!

@leahjia
Copy link

leahjia commented Dec 14, 2023

Hi @amyjko thank you for checking in. I've invited a couple of developers to work on this together and we've looked over the design, but I'll confirm by end of this week whether they would like to continue over the winter break.

@leahjia
Copy link

leahjia commented Dec 16, 2023

Update: Looks like most of us will be traveling over the holiday. I'll unassign myself.

@leahjia leahjia removed their assignment Dec 16, 2023
@momoko0719 momoko0719 removed their assignment Jan 15, 2024
@choKaylee choKaylee self-assigned this Jan 25, 2024
@amyjko
Copy link
Collaborator Author

amyjko commented Mar 15, 2024

It's the end of Winter! Please provide an update on this issue, including:

  • Whether you plan to continue work on it
  • If you do not, 1) provide a detailed update of its status, 2) add any unfinished work, 3) list any any unmerged branches, and 4) unassign yourself, so someone else can take over the task. Please leave the issue in better shape than you found it, helping future contributors to start from where you left off.

If you do plan to continue work on it, carry on. Otherwise, thank you for your contributions!

@lpjjj1222
Copy link
Contributor

Hi @amyjko ,
I have resolved the whole issue and my pull request has already been merged =)
#360

@amyjko
Copy link
Collaborator Author

amyjko commented Mar 16, 2024

Ah, it looks like you didn't include the required "Fixed #256" in a commit, so this didn't auto close. Closing :)

@amyjko amyjko closed this as completed Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buildable When an enhancement has a design ready to be built
Projects
None yet
Development

No branches or pull requests

5 participants