Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upPut "Media Only" option in column settings instead of content area headline #7801
Conversation
chr-1x
added some commits
Jun 14, 2018
marrus-sh
reviewed
Jun 15, 2018
I didn't check to see how changeColumnParams() was used before to know if this breaks anything—it looks like before, it would replace the whole params object, whereas now it is simply updating a value?? But if it was only ever used for this purpose, that shouldn't be a problem.
In the case of the right-hand (unpinned) column, you might want to do work such that if the user navigates to a …/media URL, it will flip the toggle appropriately, which it doesn't look like is the case right now. (Unless I missed something?) I made some other comments regarding that at the appropriate-ish points in the code.
Other than that, LGTM (not that my voice carries a whole lot of weight, or anything ^.^)
| + const { columnId } = this.props; | ||
| + if (!columnId && key[0] === 'other' && key[1] === 'onlyMedia') { | ||
| + this.context.router.history.replace(`/timelines/public/local${checked ? '/media' : ''}`); | ||
| + } |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
marrus-sh
Jun 15, 2018
Contributor
Given that whether to show only media is now a toggle (stored in the Redux state) rather than a tab (stored via URL state), I'm not sure that having a separate …/media URL is a good idea. (There aren't separate URLs for any other setting toggles.)
You might want to just get rid of the router context stuff and …/media URL handling, unless there is a pressing backwards-compatibility reason not to. As is, pressing the "back" button might (??!) undo the toggle (or do nothing, i'm not sure
marrus-sh
Jun 15, 2018
Contributor
Given that whether to show only media is now a toggle (stored in the Redux state) rather than a tab (stored via URL state), I'm not sure that having a separate …/media URL is a good idea. (There aren't separate URLs for any other setting toggles.)
You might want to just get rid of the router context stuff and …/media URL handling, unless there is a pressing backwards-compatibility reason not to. As is, pressing the "back" button might (??!) undo the toggle (or do nothing, i'm not sure
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
marrus-sh
Jun 15, 2018
Contributor
Actually, on thinking more about it… is the current state of the PR that Redux is being used for the toggle but the URL is being used for the filtering?? Because you should adjust it so that Redux is being used for everything if so. (Just by inserting ['params', 'other', 'onlyMedia'] into the props at the appropriate places and then reading from that instead of the URL.)
marrus-sh
Jun 15, 2018
Contributor
Actually, on thinking more about it… is the current state of the PR that Redux is being used for the toggle but the URL is being used for the filtering?? Because you should adjust it so that Redux is being used for everything if so. (Just by inserting ['params', 'other', 'onlyMedia'] into the props at the appropriate places and then reading from that instead of the URL.)
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
chr-1x
Jun 15, 2018
Contributor
Pressing the back button doesn't undo the toggle, since I'm using .replace instead of .push
However, you're right that URL state isn't a terribly elegant way to do this. When I was writing this my goal was to reach parity with the tab-based version, which meant doing things the same way it did them.
Also, at the time, I wasn't sure how to change a parameter of the mounted column, as it doesn't have a column id (so changeColumnParams wouldn't work). I think I have a better understanding of the system now though, so I'll see what I can do.
chr-1x
Jun 15, 2018
Contributor
Pressing the back button doesn't undo the toggle, since I'm using .replace instead of .push
However, you're right that URL state isn't a terribly elegant way to do this. When I was writing this my goal was to reach parity with the tab-based version, which meant doing things the same way it did them.
Also, at the time, I wasn't sure how to change a parameter of the mounted column, as it doesn't have a column id (so changeColumnParams wouldn't work). I think I have a better understanding of the system now though, so I'll see what I can do.
| + const { columnId } = this.props; | ||
| + if (!columnId && key[0] === 'other' && key[1] === 'onlyMedia') { | ||
| + this.context.router.history.replace(`/timelines/public${checked ? '/media' : ''}`); | ||
| + } |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
| const columns = state.get('columns'); | ||
| const index = columns.findIndex(item => item.get('uuid') === uuid); | ||
| - const newColumns = columns.update(index, column => column.update('params', () => fromJS(params))); | ||
| + const newColumns = columns.update(index, column => column.updateIn(['params', ...path], () => value)); |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
marrus-sh
Jun 15, 2018
Contributor
You should just be able to use setIn(['params', ...path], value) rather than updateIn() here, unless I'm mistaken??
marrus-sh
Jun 15, 2018
Contributor
You should just be able to use setIn(['params', ...path], value) rather than updateIn() here, unless I'm mistaken??
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
chr-1x
Jun 15, 2018
Contributor
I didn't check to see how changeColumnParams() was used before to know if this breaks anything—it looks like before, it would replace the whole params object, whereas now it is simply updating a value?? But if it was only ever used for this purpose, that shouldn't be a problem.
I grepped around the codebase and I'm 99% sure this was the only place it was being used. Before media only columns, nothing ever had state stored on the column itself in the redux state.
I grepped around the codebase and I'm 99% sure this was the only place it was being used. Before media only columns, nothing ever had state stored on the column itself in the redux state. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
chr-1x
Jun 15, 2018
Contributor
Just pushed an update that makes it so CommunityTimeline and PublicTimeline derive their onlyMedia property straight from the redux state.
However, having now written (slash copy/pasted) the same code four times that looks for the right column index based on the column ID and then selectively grabs parameters out of either that or the unpinned timeline settings, I think there's a little bit of refactoring to do here.
|
Just pushed an update that makes it so CommunityTimeline and PublicTimeline derive their onlyMedia property straight from the redux state. However, having now written (slash copy/pasted) the same code four times that looks for the right column index based on the column ID and then selectively grabs parameters out of either that or the unpinned timeline settings, I think there's a little bit of refactoring to do here. |
chr-1x commentedJun 14, 2018
•
edited
Edited 1 time
-
chr-1x
edited Jun 14, 2018 (most recent)
-
chr-1x
created Jun 14, 2018
This PR puts the "Media Only" option on a toggle in the settings for local and federated columns, instead of as a header that takes up valuable real estate in the column content area. It also adds a little bit of support code to the settings containers for local and federated TLs that allow settings to be saved on a per-column, rather than per-timeline-type, basis.
Justification:
Code quality note: I'm new to react / redux so a code review of the changes I've made would probably be helpful (especially around the code for the community/public TL settings, which has some cross-cutting concerns that might not be divided up in the cleanest way possible (column settings now needs to know the column ID, because the column settings container passes it a different settings object depending on whether the column ID is undefined (dynamic col) or not (particular pinned col))