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

[Content]: Store docs page #774

Open
SpencerWhitehead7 opened this issue Jun 11, 2024 · 5 comments
Open

[Content]: Store docs page #774

SpencerWhitehead7 opened this issue Jun 11, 2024 · 5 comments
Labels
planned Features or content that are planned but not yet in progress. solid Related to core Solid

Comments

@SpencerWhitehead7
Copy link

SpencerWhitehead7 commented Jun 11, 2024

📚 Subject area/topic

Store

📋 Page(s) affected (or suggested, for new content)

https://docs.solidjs.com/concepts/stores

📋 Description of content that is out-of-date or incorrect

There are opportunities to improve phrasing, clarity, organization of information, as well as correct a couple simple factual errors on the store docs page.

For a comprehensive, "code complete" accounting of possible changes, see #770

🖥️ Reproduction in StackBlitz (if reporting incorrect content or code samples)

No response

@SpencerWhitehead7
Copy link
Author

SpencerWhitehead7 commented Jun 13, 2024

As a couple representative examples of the kind of change I think could be helpful,

Copy changes like the one for this info box:

Separating the read and write capabilities of a store provides a valuable debugging advantage.

This separation facilitates the tracking and control of the components that are accessing or changing the values.

to:

Having separate read and write capabilities is a valuable code organization and debugging advantage.
// the language is more direct "separating... provides" to "having separate... is"

It lets you give components the store to read without implicitly and automatically letting them modify it.
// the original makes readers figure out why read/write separation "facilitates the tracking and control" (and what exactly does tracking or control mean in this context?)
// the altered explains WHY it's an advantage; because you can give components the ability to just read or just write without automatically granting both to every component that needs either

Organization changes like the different ways of updating a store with path syntax, going from:

  • Modifying store values
  • Path syntax flexibility
  • Modifying values in arrays
    • Appending new values
    • Range specification
    • Dynamic value assignment
    • Filtering values
  • Modifying objects

to

  • Modifying store values
  • Providing the new value
  • Modifying objects
  • Modifying arrays
    • Updating one element by index
    • Updating multiple elements by index
    • Updating multiple elements with an index range
    • Updating elements conditionally

The new organization gives you a top level overview of modifying values and the basics of how path syntax works, as well as how to provide new values for the setter. Then it goes into each of the different specific path syntaxes you can use, from least to most complicated.

The original does not explicitly explain how to provide a new static value, and leaves the reader to piece it together from the examples. It's pretty straightforward, so it's not a big deal, but it could be a bit clearer. The original does explicitly explain how to provide a new value dynamically, but organizationally, that's been nested under "Modifying arrays," even though it's works no matter what path syntax you use to reach the target (object or array). Both ways of providing a new value should be explicit and they should have their own section.

"Appending new values" is actually an explanation of how to modify values by index; appending is just a special case of that general syntax.

I don't think the new organization is necessarily indisputable or perfect (you could probably fold the "Updating by Index" sections together and just provide two examples, one for individual and one for multiple, and "Updating multiple elements with an index range" should probably just be "Updating elements by index range", since the multiple is understood from "elements" instead of "element" and it's shorter).

Factual fixes, like:
With path syntax, you can target a subset of elements to update or modify by specifying a range of indices. You can do this using an array of values: Providing an array of indexes selects the items at those indexes, not the range between the indexes provided in the array. The wording's a little ambiguous; it's technically correct if you read "range" to just mean "multiple" instead of the more commonsense meaning, but either way it's made unambiguous in the updated version.

or:
If your store is array, you can specify a range of indices using an object with from and to keys. this just isn't true. The range syntax will work on nested array fields of stores too, the store itself doesn't have to be an array.

I could write out an equally or more detailed justification for every altered line in the PR, but I hope this'll serve as a representative example of the kind of thing I meant by opportunities to improve phrasing/organization/factual fixes.

@LadyBluenotes
Copy link
Member

Hi Spencer and thanks for filing this issue.

I'm struggling to understand what about the Stores docs requires the rewrite you had proposed. While I don't disagree that there are some parts that could be better addressed, you have mentioned that the phrasing, clarity, and organization need to be improved but I'm not sure where you're referring to and your PR is touching almost every aspect of the page which makes it difficult to follow.

With that being said, I appreciate the PR you sent in and I understand it must have taken a lot of time and effort to do it. However, like I mentioned, linking it here doesn't quite help me understand what changes you're looking for. In addition, the examples you have brought up above appear to be more related to how a specific sentence was structured versus the content itself.

With that being said, this time I did go to your PR and I have some comments / questions:

  • What about the organization of the page do you find confusing? Do you find the existing headings difficult to follow or understand? I notice you changed almost every heading, some by just rearranging words - why do you feel like these changes were needed and what about the existing ones do you feel were lacking?
  • I saw you mentioned that there was no section specifically dedicated to new values and I agree that it could use an explicit callout.

As for the rest of it, there are a few ways we can address this page but I mainly think that breaking it up would be more helpful. For one, since we agree that a section to explicitly address how to add new values to a store could be useful, we could take that out of this issue and put it in its own. That way a PR can be worked on sooner vs waiting for us to hammer out the rest of the details.

Additionally, for any errors you had found, it would be great to have those addressed in a dedicated issue of their own so we don't loop them into this and they don't end up getting held up.

Following that, I think it might be worthwhile to break up what you're looking for into more bite-sized chunks, whether through issues or just better calling things out within this one. (It makes it a lot easier for me to follow what you are looking for).

As an example of what could be more helpful is something like the following (this is just an example, by the way, please explain why you feel like the rework is necessary so I can comment on things a bit more directly):

I feel like using words like can (like the first sentence under Accessing store values ) introduces unnecessary mental overhead for users (ie. are there other ways you can do it outside of direct references?).

-or-

The paragraph under the first example of Accessing store values is adding context to something that I don't feel like is needed.

Let me know if you have any questions regarding what I've said. That being said, I look forward to working together to get this page into a better spot :)

@SpencerWhitehead7
Copy link
Author

I had a much longer reply before but I think a tldr would be more helpful.

Like you say, most of the changes are just rewordings and copyediting, the same good material expressed differently. Those are the changes I'm looking for. It's difficult to express them as an issue because it's just a million individually tiny copyediting things. They clarify the language used to make the page more beginner friendly/comprehensible without sacrificing the knowledge and depth of the current version. Opening an issue that I didn't find the page very understandable when I read it as a first time store user is just a demand that you do a bunch of dull, time consuming, relatively low value copyediting and revision work, but actually doing the work myself is (I hope) helpful and requires a relatively small lift to use.

For changes to the page organization, rather than page copy, the only part I had trouble with was the Modifying store values section, and that's the only section where things have actually been added/removed/reordered. As described in the previous comment, it adds a section on how to provide a new value and reorders the other sections from least to most complicated so that they build on each other. Changes to the text of the section titles:

  • Modifying values in arrays -> Modifying arrays
    • the original explicitly says it's about modifying values in arrays. But the content also explains ways to modify the array itself. The updated copy is shorter and encompasses both things the section actually explains.
  • Range specification -> Updating multiple elements with an index range
    • all the subsections under Modifying arrays have a consistent structure; this makes it clearer that they're all just different examples of the same general thing (path syntax for arrays).
    • "Range specification" is not descriptive; what does that mean in this context? Updating multiple elements with an index range tells you exactly what the section describes; how to update multiple elements based on a range of indexes.
  • Filtering values -> Updating elements conditionally
    • this one is just a misnomer. The section does not tell you how to filter values out of an array. It tells you how to select only certain elements to modify based on a dynamic condition (as opposed to by index or index range). The new title doesn't capture the full nuance or complexity, but I think it gets at the basic idea and at least it's not misleading.
  • Store updates with produce -> Updating data with produce
  • Data integration with reconcile -> Integrating data with reconcile
  • Extracting raw data with unwrap -> Extracting data with unwrap
    • These are the smallest changes, it just aligns them to all have the same structure. This emphasizes that all three utilities help you deal with the data inside the stores, specifically, produce helps you update data, reconcile lets you integrate new data, and unwrap lets you extract data.

I don't think writing a justification for why each editing-type change is desirable would be a productive use of time. Describing a prose change with... more prose doesn't seem helpful since the versions can already be directly compared and it takes 10x longer to produce a thorough, well written explanation than it does to do the edit. If you review it and there's information I dropped, misinformation I added, or confusing/misleading language changes I'll be happy to fix it together.

As a way to make it more reviewable, how about I work on breaking the PR into multiple commits that scope down the changes and make it easier to see what each one does?

@SpencerWhitehead7
Copy link
Author

Hi @LadyBluenotes , should I proceed with splitting it into multiple commits?

@LadyBluenotes
Copy link
Member

Hi @SpencerWhitehead7 - for the time being I think we're going to put this on hold. Once we are in a place where this can be appropriately addressed, I'll reach out!

@LadyBluenotes LadyBluenotes added planned Features or content that are planned but not yet in progress. and removed improve documentation pending review Awaiting review by team members. labels Jul 18, 2024
@LadyBluenotes LadyBluenotes removed their assignment Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
planned Features or content that are planned but not yet in progress. solid Related to core Solid
Projects
None yet
Development

No branches or pull requests

2 participants