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

Archive channels #305

Merged
merged 4 commits into from
Mar 28, 2021
Merged

Archive channels #305

merged 4 commits into from
Mar 28, 2021

Conversation

nikolaiwarner
Copy link
Member

Screen Shot 2021-03-16 at 8 50 35 AM

Screen Shot 2021-03-16 at 8 54 32 AM

This also bumps Cabal Desktop to use the latest cabal-client and does just a little code cleanup.

app/actions.js Outdated
Comment on lines 54 to 58
// Disable a few slash commands for now
const removedCommands = ['add', 'channels', 'clear', 'ids', 'names', 'new', 'qr', 'whoami', 'whois']
removedCommands.forEach((command) => {
client.removeCommand(command)
})
// const removedCommands = ['add', 'channels', 'clear', 'ids', 'names', 'new', 'qr', 'whoami', 'whois']
// removedCommands.forEach((command) => {
// client.removeCommand(command)
// })
Copy link
Member

@cblgh cblgh Mar 16, 2021

Choose a reason for hiding this comment

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

should we just remove these lines?

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe?! this was among the few things that didnt work with the latest cabal-client. i didnt spend any time trying to understand why though.

Copy link
Member

Choose a reason for hiding this comment

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

ah! maybe that's my fault when i refactored the commands stuff! let's create an issue and link it at the commented out code block?

app/actions.js Outdated Show resolved Hide resolved
app/actions.js Show resolved Hide resolved
@@ -343,7 +381,7 @@ export const viewChannel = ({ addr, channel, skipScreenHistory }) => (dispatch,
client.focusChannel(channel)
client.markChannelRead(channel)
} else {
dispatch(joinChannel({ addr, channel }))
// dispatch(joinChannel({ addr, channel }))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// dispatch(joinChannel({ addr, channel }))

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the main issue that came up with the cabal-client update which threw the app into a loop. It seems that joinChannel isnt really needed here though as things seem to work as expected without it. My guess is other bugs in Desktop were hiding the real problem.

Copy link
Member

Choose a reason for hiding this comment

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

maybe we can encode this in a comment above the line? would be nice to keep the context intact, in case there's a bug in cabal-client somehow :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok!

app/actions.js Show resolved Hide resolved
@cblgh
Copy link
Member

cblgh commented Mar 16, 2021

@nikolaiwarner nice! 🍰 i think there is more margin than needed between the leave channel / archive channel buttons in the first screenshot, do you agree with making it a bit tighter?

Co-authored-by: Alexander Cobleigh <[email protected]>
@nikolaiwarner
Copy link
Member Author

@nikolaiwarner nice! 🍰 i think there is more margin than needed between the leave channel / archive channel buttons in the first screenshot, do you agree with making it a bit tighter?

thanks! yeah, i also think i should swap leave and archive. since one may leave more often than archive.

i'm in a weird place with this codebase really. i'm quite ready to start from scratch... approaching this project with what we've learned over three years, meets our current goals, and does things in the most modern and efficient way. that of course would be the project that builds the desktop client and the interface for browsers. so i guess what im saying is that this might be the last feature until cabal-desktop-next

app/actions.js Show resolved Hide resolved
Co-authored-by: Alexander Cobleigh <[email protected]>
@cblgh
Copy link
Member

cblgh commented Mar 16, 2021

i'm in a weird place with this codebase really. i'm quite ready to start from scratch... approaching this project with what we've learned over three years, meets our current goals, and does things in the most modern and efficient way.

i hear ya :)

that of course would be the project that builds the desktop client and the interface for browsers. so i guess what im saying is that this might be the last feature until cabal-desktop-next

💯 i think it's at the perfect stopping off point for new features as well, considering the timing of starting on cabal-web. i think the exception would be tiny bug fixes for people using the clients in the interim? i'm mainly thinking of finding a fix for messages showing up twice. that's easy for me to say tho, considering i mostly pull my weight in the other repos 😅

anyway, nice!!! it would be cool if we could add the context you wrote in the comments above inside the source as code comments, other than that 🚢 📦 it! 🖤

@nikolaiwarner
Copy link
Member Author

I'll clean things up with your suggestions then make a build that i'll share privately with some folx. I'd like to test things out real good before merging this and releasing.

@cblgh
Copy link
Member

cblgh commented Mar 16, 2021

I'll clean things up with your suggestions then make a build that i'll share privately with some folx. I'd like to test things out real good before merging this and releasing.

heck yeah! i'll give it a spin on my win desktop some time this week then :)

@nikolaiwarner nikolaiwarner merged commit 9a5d0ed into master Mar 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants