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

Make id optional #114

Closed
risalfajar opened this issue Mar 12, 2023 · 3 comments · Fixed by #118
Closed

Make id optional #114

risalfajar opened this issue Mar 12, 2023 · 3 comments · Fixed by #118

Comments

@risalfajar
Copy link
Contributor

Can we just make id in table.column() with function accessor and table.display() optional? Since header is already unique in most cases, we can use that as default value for id.

@bryanmylee
Copy link
Owner

The only issue is that id needs to be unique because it's used for tracking columns within each group and column ordering / hiding.

header is unique in most cases, but it's also possible to have a non-text header. If we make id optional, we'd have to add a lot of complexity to the system.

@risalfajar
Copy link
Contributor Author

Wouldn't adding more checks in DataColumn constructor suffice?

Something like:

	constructor({
		header,
		footer,
		plugins,
		cell,
		accessor,
		id,
	}: DataColumnInit<Item, Plugins, Id, Value>) {
		super({ header, footer, plugins, id: 'Initialization not complete' });
		this.cell = cell;
		if (accessor instanceof Function) {
			this.accessorFn = accessor;
		} else {
			this.accessorKey = accessor;
		}
		if (id === undefined && this.accessorKey === undefined && header === undefined) {
			throw new Error('A column id or string accessor or header is required');
		}
		this.id = (id ?? String(this.accessorKey) ?? String(header)) as Id;
	}

@bryanmylee
Copy link
Owner

That could work! Unfortunately, I've been extremely busy so this project has been on the back-burner for me.

Could you help open a PR for it? I would love to merge any PR you have if you could provide one.

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 a pull request may close this issue.

2 participants