-
-
Notifications
You must be signed in to change notification settings - Fork 833
Conversation
edb9469
to
855aee0
Compare
855aee0
to
9e4d8e7
Compare
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.
we should document this in the riot-web README (as per element-hq/element-web#6791) instead of the develop docs
@turt2live How about if we document it here and in riot-web? |
I have a fear that they'd get out of sync or forgotten about. Generally I try and consider riot-web to be the config side of things (despite the config being defined here) and the react-sdk being the logic layer. I don't really mind if it's both, but at a minimum it should be in riot-web to help users. |
Hmm, I guess I am confused... The PR here isn't trying to document each setting... It's just trying to mention that settings can be defaulted, which doesn't seem so likely to get out of sync, right? It's not likely to change unless we overhaul settings again. |
bleh, yes, that's true. The fact that the object exists though still makes most of my argument make sense, I think. The idea being that "hey, a flag exists for things" should be documented to riot admins and less so developers (ie: dump info in riot-web for riot admins, developer docs in react-sdk). |
This one seems to have been forgotten by me. I would go ahead and merge this if nobody objects as this is also already documented in |
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.
lgtm otherwise
Sorry, this showed up at the bottom of my review queue which is often reserved for things that I'm ignoring or putting off ._.
@bwindels can we get this over the line :D? |
Co-Authored-By: Travis Ralston <travpc@gmail.com>
Co-Authored-By: Travis Ralston <travpc@gmail.com>
Not sure why all these tests are failing, but I am fairly sure it has nothing to do with these documentation changes, so merging. |
See element-hq/element-web#6791 & element-hq/element-web#8449
Also element-hq/element-web#9919