feat: change workspace names to icons #1240

orgrimarr · 2023-06-15T21:39:58Z

Pre-flight Checklist

Please ensure you've completed all of the following.

I have read the Contributing Guidelines for this project.
I agree to follow the Code of Conduct that this project adheres to.

Description of Change

I reduced the size of the workspaceDrawer
I replaced the workspaceItem text with an icon or the first letter. as a fallback
I added in the workspace config a icon field in the form

The icon field is not persisted, I dit not manage to do it.
The icon config could be more user-friendly than a simple text input

Motivation and Context

This PR is a proposal regarding this issue #139

Screenshots

Checklist

My pull request is properly named
[x]The changes respect the code style of the project (pnpm prepare-code)
pnpm test passes
I tested/previewed my changes locally

Release Notes

vraravam · 2023-06-16T01:51:41Z

@orgrimarr - is there a reason for this to be in "Draft" mode still?

vraravam

Great initiative! Please continue with this, I will close my draft PR

src/components/ui/WorkspaceIcon.tsx

src/features/workspaces/components/WorkspaceDrawer.tsx

src/themes/default/index.ts

vraravam · 2023-06-16T02:02:19Z

Please create your branch from the latest version of the upstream's main/master (in this case develop) branch
When contributing to OSS, please check if other PRs are open for the same feature by someone else. Maybe if one exists, then you can collaborate in the same PR as opposed to creating a competing implementation.

orgrimarr · 2023-06-16T05:44:38Z

Please create your branch from the latest version of the upstream's main/master (in this case develop) branch

When contributing to OSS, please check if other PRs are open for the same feature by someone else. Maybe if one exists, then you can collaborate in the same PR as opposed to creating a competing implementation.

Hi,

Thanks for your fast review, 😀

The goal was not to create a competing implementation, I just thought is was an easy way to share code.
That why this PR is still in draft mode.

Sorry for that 😕

orgrimarr · 2023-06-16T06:20:26Z

Some questions 😀 :

In order allow both visual, should I add an option in the sidebar setting like "Use workspace drawer icon style" ?
In the text mode, should we display an icon ? or only if configured ? or not ?, if yes then the setting might be "Hide workspace drawer text"

How can I persist theses settings ? (icon path, drawer visual) ?
I din't understand what should I do in the code

I'm only using Ferdium without an account. I wonder how to sync the icon setting. Because if we use a local path, the image might not exist in another host, and the OS path system might be different. Should we store the image content ? (We can ask for a specific image format and size like Rambox to reduce the size to a acceptable one)

vraravam · 2023-06-16T09:11:08Z

In order allow both visual, should I add an option in the sidebar setting like "Use workspace drawer icon style" ? In the text mode, should we display an icon ? or only if configured ? or not ?, if yes then the setting might be "Hide workspace drawer text"

Add this user preference in the workspace preference screen. I would also move the icon text box outside of the "create" action, and into the screen where the services are associated with a workspace.

How can I persist theses settings ? (icon path, drawer visual) ? I din't understand what should I do in the code
I'm only using Ferdium without an account. I wonder how to sync the icon setting. Because if we use a local path, the image might not exist in another host, and the OS path system might be different. Should we store the image content ? (We can ask for a specific image format and size like Rambox to reduce the size to a acceptable one)

The service and similarly the workspace preferences are ultimately stored in a sqlite db - whether its using the "accountless" option (which is basically running a local server), or the hosted server solution. The internal servers' code changes are in the same repo, but the hosted server is in a sibling repo called ferdium-server. I can help review/point you to the code file where the DML needs to be changed to add a new column in that corresponding table.

orgrimarr · 2023-06-17T09:31:06Z

Thanks :)

I will add a button in the user preference 😀

I would also move the icon text box outside of the "create" action, and into the screen where the services are associated with a workspace.

What do you mean by 'move the icon text box outside of the "create" action' ?
Should I remove the icon input in the create view like this ?
So the icon can only be edited where the services are associated with a workspace

I can help review/point you to the code file where the DML needs to be changed to add a new column in that corresponding table.

Yes please, can you help me on that point ? Which file should I modify to add

The new icon property on the Workspace data
The new user setting property to toggle the icon view

orgrimarr · 2023-06-17T13:43:38Z

It should work as discussed.

Just missing the workspace iconUrl persistance.
The user setting seams to be automagically persisted 😀

vraravam · 2023-06-17T14:56:13Z

src/components/layout/AppLayout.tsx

@@ -53,12 +53,21 @@ const styles = (theme: { workspaces: { drawer: { width: any } } }) => ({
    // width: `calc(100% + ${theme.workspaces.drawer.width}px)`,
    width: '100%',
    transition,
+  },
+  appContentTransformTextWorkspace: {
    transform() {
      return workspaceStore.isWorkspaceDrawerOpen


Would it be better to move the ternary to be evaluated inside the translateX() like so?

return 'translateX(`workspaceStore.isWorkspaceDrawerOpen ? 0 : -75px`)');

(Syntax might be incorrect since I'm not typing inside an editor)

Sure, we can do that

vraravam · 2023-06-17T14:56:53Z

src/components/layout/AppLayout.tsx

    transform() {
      return workspaceStore.isWorkspaceDrawerOpen
        ? 'translateX(0)'
        : `translateX(-${theme.workspaces.drawer.width}px)`;
    },
  },
+  appContentTransformIconWorkspace: {
+    transform() {
+      return workspaceStore.isWorkspaceDrawerOpen


I'm not sure that it is more readable.
If it's ok for you, I can push that change

appContentTransformTextWorkspace: { transform() { return `translateX(${ workspaceStore.isWorkspaceDrawerOpen ? '0' : `-${theme.workspaces.drawer.width}px` })`; }, }, appContentTransformIconWorkspace: { transform() { return `translateX(${ workspaceStore.isWorkspaceDrawerOpen ? '0' : `-75px` })`; }, },

vraravam · 2023-06-17T14:59:11Z

src/containers/settings/EditSettingsScreen.tsx

@@ -259,6 +259,10 @@ const messages = defineMessages({
    id: 'settings.app.form.alwaysShowWorkspaces',
    defaultMessage: 'Always show workspace drawer',
  },
+  useWorkspaceDrawerIconStyle: {
+    id: 'settings.app.form.useWorkspaceDrawerIconStyle',
+    defaultMessage: 'Use workspace drawer icon style',


So, I was suggesting to have the user choose whether they want the name or the first character of the workspace name AND/OR the icon. Could the feature be imagined like that?

If I understand correctly.

The letter is currently a fallback for workspaces that don't have an icon.

The setting should be a select with theses choices :

text (current display)

(letter OR icon) AND text

letter AND icon (if icon configured)

letter OR icon (current PR)

It means, 3 different workspace drawer sizes

the current (300px by default)

icon OR letter only (35 - 75px)

letter + icon (let's say 75 - 150px)

Should the workspace drawer bar be the same size of the service bar ?
I means use the serviceRibbonWidth parameter for both workspace and service bar (only when displaying icon)

@vraravam Can you confirm that I have understood correctly ?
If yes, I will start the dev this week 😀

we don't need to complicate so much. only 2 options that i was suggesting:

workspace name (current)

icon or first character of workspace name (if the user wants to use a smaller-width option for the workspace drawer)

To permanently save this field you would probably need to sync with the server (internal and external) if I'm correct.

I'm not recalling if the server is saving workspace data as a plain JSON or not (only by checking the endpoint we can be sure of that). If that is not the case, you would need to touch both the server and the internal-server... but with the ongoing Adonis update this could be a hassle :/ I can help on that tho.

Also, can we make this option the default one? I like this new look (even though I'm not using workspaces).

Great work!!

yes, you will need to sync to the internal and external servers. The db schema is the same across both - and there's a table called workspaces to store this info.

As seen in the above pic (taken from my internal server), you can either add a new column for the useIcon or you can add it into the data column which holds JSONB datatype. (I personally prefer option 1.

yes, you will need to sync to the internal and external servers. The db schema is the same across both - and there's a table called workspaces to store this info.

As seen in the above pic (taken from my internal server), you can either add a new column for the useIcon or you can add it into the data column which holds JSONB datatype. (I personally prefer option 1.

Even though I would prefer option 1 (just like you) I would advise into adding it to "data" column (but first lets be sure what type of data is being retrieved there) so that it doesn't make the migration to the new Adonis's even more complicated (as we would to update both the code in the current ferdium-server, internal server and on the current adonis update PR)

As suggested, i added the iconUrl field in data

WIP related PR on the server : ferdium/ferdium-server#102

src/i18n/locales/fr.json

SpecialAro · 2023-09-05T23:02:39Z

Hi @orgrimarr I've been testing your code and it seems that it breaks functionality if you use an horizontal bar and not a vertical one. Can you please fix that?

Also, I've take a look at how we update workspaces and, indeed, we are going to need to touch both the internal server and Ferdium server after all (no escape from that). So there is that... I can help you with that tho!

orgrimarr · 2023-09-06T06:10:14Z

Thanks you both for your comments !
I will fix this bug and will modify the server 😃

ferdium#1240 (comment)

ferdium/ferdium-app#1240

ferdium#1240 (comment)

vraravam · 2024-02-11T05:03:15Z

@orgrimarr - is this PR mergeable at least for the internal server bits which live in this codebase itself? Can you please help to take this to completion?

vraravam · 2024-02-12T23:19:18Z

Thanks for the confirmation @orgrimarr

@SpecialAro - we currently support 4 server backends:

Franz
Ferdi
Ferdium (hosted)
no server

This change is going to break for the first 2 anyways. The 3rd one, while under our control, still has a ways to go. With this being the case, how shall we proceed?

vraravam · 2024-02-13T02:48:26Z

I notice that we can also use icon url

yes - I would prefer this option as opposed to uploading an icon. For the rare cases where folks create and use custom icons, they can self-host in some website, and provide that url. In most cases though, icons are usually available on the net and so a url is easier for us to keep track of/maintain that a binary file. This will also reduce the burden on syncing between the different server backends.

@orgrimarr - could you please make such a change within this same PR?

ferdium#1240 (comment)

orgrimarr · 2024-02-14T12:27:27Z

I notice that we can also use icon url

yes - I would prefer this option as opposed to uploading an icon. For the rare cases where folks create and use custom icons, they can self-host in some website, and provide that url. In most cases though, icons are usually available on the net and so a url is easier for us to keep track of/maintain that a binary file. This will also reduce the burden on syncing between the different server backends.

@orgrimarr - could you please make such a change within this same PR?

@vraravam I think no changes are required.

Currently, we can only configure strings https://github.com/ferdium/ferdium-app/pull/1240/files#diff-e8933cda65f8319a0a3982de972254fd0ef8f96dac31e0530660e5608682ef3bR10

url
local file path

This property is used to configure the image src https://github.com/ferdium/ferdium-app/pull/1240/files#diff-46ab6517cb2faf44c8255f0b1a0955a5b534eaa2786fc1f1fbbe97e85d16942cR51

SpecialAro · 2024-02-14T12:42:47Z

Thanks for the confirmation @orgrimarr

@SpecialAro - we currently support 4 server backends:

Franz

Ferdi

Ferdium (hosted)

no server

This change is going to break for the first 2 anyways. The 3rd one, while under our control, still has a ways to go. With this being the case, how shall we proceed?

The first two (I think the 2nd one we are not supporting anymore) are going to break regardless - and users are aware of this (a message pops-up when they change to the first two. But, given that we are referring to the same project (Ferdium) I have the opinion that we should make this feature work across our entire ecosystem (account and accountless).

In fact, if this works for the internal server (which still lacks the adonis update) then it should work for ferdium server, given that the implementation is rather similar (almost copy paste on the code).

orgrimarr · 2024-02-14T13:05:11Z

@vraravam @SpecialAro Regarding the url persistance within the Workspace table.

Now that the adonis migration is done, should i keep saving the url in the data column ? or create a new one ?

I can try to modify ferdium-server this week

vraravam · 2024-02-14T13:28:03Z

Now that the adonis migration is done, should i keep saving the url in the data column ? or create a new one ?

Keeping the same schema will hopefully keep us backward compatible with all the currently-supported servers (franz, ferdi, ferdium hosted, no-server). So I would not change that db schema at this point.

vraravam · 2024-02-14T13:30:06Z

Currently, we can only configure strings https://github.com/ferdium/ferdium-app/pull/1240/files#diff-e8933cda65f8319a0a3982de972254fd0ef8f96dac31e0530660e5608682ef3bR10

yes, the datatype in the codebase is a string - but, at the UI level, I think its expects a file to be dragged and dropped. How will the end user experience be if they want or are forced to use a url?

orgrimarr · 2024-02-14T16:08:49Z

Currently, we can only configure strings https://github.com/ferdium/ferdium-app/pull/1240/files#diff-e8933cda65f8319a0a3982de972254fd0ef8f96dac31e0530660e5608682ef3bR10

yes, the datatype in the codebase is a string - but, at the UI level, I think its expects a file to be dragged and dropped. How will the end user experience be if they want or are forced to use a url?

At the UI level it look like this

Maybe i can iprove the placeholder with something like "Icon url" or "Icon url or local path"

vraravam · 2024-02-14T16:20:10Z

yes, a more explicit text would be very much appreciated. Sorry, I was confusing with the service icon, then realized you are referring to the workspace icon.

ferdium/ferdium-app#1240

ferdium#1240 (comment)

ferdium/ferdium-app#1240

vraravam · 2024-05-30T01:39:04Z

@orgrimarr - could you please resolve the conflicts?

@orgrimarr / @SpecialAro : is this PR ready to be merged (once the above is done)?

vraravam · 2024-06-06T12:59:13Z

Closing due to inactivity. Its been 1 year since the PR was opened, and no response from the OP.

orgrimarr · 2024-06-07T06:51:30Z

Hi @vraravam this PR is ready for merge. (Just need to rebase and fix conflicts as you asked)

There were some discussion on the server / database side regarding backward compatibility : ferdium/ferdium-server#102

vraravam · 2024-06-07T09:53:22Z

Hi @vraravam this PR is ready for merge. (Just need to rebase and fix conflicts as you asked)

There were some discussion on the server / database side regarding backward compatibility : ferdium/ferdium-server#102

please rebase, and then we can reopen the PR.

vraravam assigned orgrimarr Jun 16, 2023

vraravam force-pushed the workspace-bar-icons branch from add9773 to 722fb80 Compare June 16, 2023 01:51

vraravam requested changes Jun 16, 2023

View reviewed changes

src/components/ui/WorkspaceIcon.tsx Outdated Show resolved Hide resolved

src/features/workspaces/components/WorkspaceDrawer.tsx Outdated Show resolved Hide resolved

src/themes/default/index.ts Outdated Show resolved Hide resolved

orgrimarr force-pushed the workspace-bar-icons branch from 722fb80 to cccd509 Compare June 17, 2023 13:40

orgrimarr marked this pull request as ready for review June 17, 2023 13:41

orgrimarr requested a review from a team as a code owner June 17, 2023 13:42

vraravam reviewed Jun 17, 2023

View reviewed changes

vraravam force-pushed the workspace-bar-icons branch from cccd509 to a9fd34f Compare June 17, 2023 15:03

vraravam force-pushed the workspace-bar-icons branch from a9fd34f to e901ed6 Compare August 15, 2023 23:34

vraravam force-pushed the workspace-bar-icons branch from e901ed6 to f84e89f Compare September 5, 2023 02:51

SpecialAro reviewed Sep 5, 2023

View reviewed changes

src/i18n/locales/fr.json Outdated Show resolved Hide resolved

orgrimarr added a commit to orgrimarr/ferdium-app that referenced this pull request Sep 14, 2023

feat: useWorkspaceDrawerIconStyle default to true

eb9eb11

ferdium#1240 (comment)

orgrimarr added a commit to orgrimarr/ferdium-server that referenced this pull request Sep 14, 2023

feat: add iconUrl storage

bac60c1

ferdium/ferdium-app#1240

orgrimarr mentioned this pull request Sep 14, 2023

feat: add iconUrl storage ferdium/ferdium-server#76

Closed

vraravam force-pushed the workspace-bar-icons branch from 375d246 to 7106504 Compare February 10, 2024 07:06

vraravam pushed a commit to orgrimarr/ferdium-app that referenced this pull request Feb 10, 2024

feat: useWorkspaceDrawerIconStyle default to true

c66edb9

ferdium#1240 (comment)

vraravam force-pushed the workspace-bar-icons branch from 7106504 to 9bb75ac Compare February 11, 2024 05:02

vraravam pushed a commit to orgrimarr/ferdium-app that referenced this pull request Feb 11, 2024

feat: useWorkspaceDrawerIconStyle default to true

9cd44fd

ferdium#1240 (comment)

vraravam pushed a commit to orgrimarr/ferdium-app that referenced this pull request Feb 13, 2024

feat: useWorkspaceDrawerIconStyle default to true

9766e3f

ferdium#1240 (comment)

vraravam force-pushed the workspace-bar-icons branch from 9bb75ac to af26759 Compare February 13, 2024 02:48

orgrimarr added a commit to orgrimarr/ferdium-server that referenced this pull request Feb 14, 2024

feat: Store iconUrl in workspace.data

bc6eb86

ferdium/ferdium-app#1240

orgrimarr mentioned this pull request Feb 14, 2024

feat: Store iconUrl in workspace.data ferdium/ferdium-server#102

Open

orgrimarr and others added 9 commits February 14, 2024 21:08

feat: add useWorkspaceDrawerIconStyle setting

878facc

feat: add workspace drawer icon display

0f651e8

chore: add fr lang trads

a1cd65b

chore: handle isWorkspaceDrawerOpen transform

8278516

fix: useWorkspaceDrawerIconStyle with useHorizontalStyle

3e07807

feat: useWorkspaceDrawerIconStyle default to true

099a3c4

ferdium#1240 (comment)

feat: persist iconUrl to internal-server

11095d1

fix: Save name in data on workspace edit

ac1eea9

feat: use explicit icon labels

d5794b3

vraravam force-pushed the workspace-bar-icons branch from 777ad0a to d5794b3 Compare February 14, 2024 21:08

SpecialAro pushed a commit to orgrimarr/ferdium-server that referenced this pull request Feb 15, 2024

feat: Store iconUrl in workspace.data

4f91906

ferdium/ferdium-app#1240

vraravam pushed a commit to orgrimarr/ferdium-server that referenced this pull request Feb 18, 2024

feat: Store iconUrl in workspace.data

95b3939

ferdium/ferdium-app#1240

vraravam closed this Jun 6, 2024

feat: change workspace names to icons #1240

feat: change workspace names to icons #1240

Conversation

orgrimarr commented Jun 15, 2023

Pre-flight Checklist

Description of Change

Motivation and Context

Screenshots

Checklist

Release Notes

vraravam commented Jun 16, 2023

vraravam left a comment

Choose a reason for hiding this comment

vraravam commented Jun 16, 2023

orgrimarr commented Jun 16, 2023

orgrimarr commented Jun 16, 2023

vraravam commented Jun 16, 2023 • edited Loading

orgrimarr commented Jun 17, 2023

orgrimarr commented Jun 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orgrimarr Jun 17, 2023 • edited Loading

Choose a reason for hiding this comment

orgrimarr Jun 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orgrimarr Sep 14, 2023 • edited by vraravam Loading

Choose a reason for hiding this comment

SpecialAro commented Sep 5, 2023 • edited Loading

orgrimarr commented Sep 6, 2023

vraravam commented Feb 11, 2024

vraravam commented Feb 12, 2024

vraravam commented Feb 13, 2024

orgrimarr commented Feb 14, 2024

SpecialAro commented Feb 14, 2024

orgrimarr commented Feb 14, 2024

vraravam commented Feb 14, 2024

vraravam commented Feb 14, 2024

orgrimarr commented Feb 14, 2024 • edited Loading

vraravam commented Feb 14, 2024

vraravam commented May 30, 2024

vraravam commented Jun 6, 2024

orgrimarr commented Jun 7, 2024

vraravam commented Jun 7, 2024

vraravam commented Jun 16, 2023 •

edited

Loading

orgrimarr Jun 17, 2023 •

edited

Loading

orgrimarr Jun 17, 2023 •

edited

Loading

orgrimarr Sep 14, 2023 •

edited by vraravam

Loading

SpecialAro commented Sep 5, 2023 •

edited

Loading

orgrimarr commented Feb 14, 2024 •

edited

Loading