Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

Add a text id to allow a user specified key to be used to find/organise tabs #86

Closed
wants to merge 1 commit into from

Conversation

LiamBest
Copy link
Contributor

Add a text id to allow a user specified key to be used to find/organise tabs, also update readme with new function/attributes

@brrd
Copy link
Owner

brrd commented Nov 18, 2019

IMO this feature sounds too specific to be merge into the core library. Users can already implement this behavior in an application very easily by using electron-tabs current API:

const tabs = {};
const tabGroup = new TabGroup();

const addTab = (textId, tabOptions) => {
  const tab = tabGroup.addTab(tabOptions);
  tabs[textId] = tab;
};

addTab("hello", {
    title: "Hello world",
    src: "..."
});

// => tab is now refrenced in tabs.hello

Can you give an use case where the above solution would not work?

@LiamBest
Copy link
Contributor Author

That would definitely solve my issue and avoiding changing the core, however personally it feels like a common use case to want to want to assign a key to retrieve specific tabs.
I would suggest if this is not merged we consider adding your example in the documentation for future users.

@brrd
Copy link
Owner

brrd commented Nov 18, 2019

I'm glad this solves your issue.

I would suggest if this is not merged we consider adding your example in the documentation for future users.

I think this is already clear enough in the documentation:

tabGroup.addTab(options)
Add a new tab to the tab group and returns a Tab instance.

Once you got the tab, you can store and manipulate it like any other object in JavaScript, it is a very common pattern. My previous example maybe fits to your own needs, but there are dozens of other ways to organize an application and I do not want to force users to use one in particular.

So I'm sorry but I prefer not to add this change into the documentation. But thank you anyway for your contribution.

@brrd brrd closed this Nov 18, 2019
@LiamBest
Copy link
Contributor Author

Ah, ran into one problem with the approach, the events being fired return the tab object which has no context for the textId now.

@brrd
Copy link
Owner

brrd commented Nov 19, 2019

Then store the id instead of the tab:

const tabs = {};
const tabGroup = new TabGroup();

const addTab = (textId, tabOptions) => {
  const tab = tabGroup.addTab(tabOptions);
  tabs[tab.id] = textId;
};

// Event
tab.on("active", (tab) => { 
  const id = tab.id;
  const textId = tabs[id];
});

// Get the tab from textId
const getTabFromTextId = (textId) => tabs.find(textId);
const myTab = getTabFromTextId("myTextId");

@brrd
Copy link
Owner

brrd commented Nov 19, 2019

And anyway, if you really want to add this attribute directly to your tabs because this makes you feel more comfortable then you can simply do this:

const tab = tabGroup.addTab(tabOptions);
tab.textId = "myTextId";

tab.on("active", (tab) => { 
  const textId = tab.textId;
});

This should work (although this is not a very good practice IMO).

@LiamBest
Copy link
Contributor Author

Awesome, I'll go with the approach of storing the ID for my own reference, thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants