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

Update device-type pages to provide their own titles #1809

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

blammit
Copy link
Contributor

@blammit blammit commented Dec 19, 2024

When pushing a page that uses a device name for the page title, have each page provide this title internally (using the device name) instead of passing the title in as a fixed name.

This way, the page title is updated if the device name changes (e.g. if the custom name changes) and also we don't need to duplicate the code that passes in the page title as a parameter when pushing the page.

When pushing a page that uses a device name for the page title, have
each page provide this title internally (using the device name)
instead of passing the title in as a fixed name.

This way, the page title is updated if the device name changes (e.g.
if the custom name changes) and also we don't need to duplicate the
code that passes in the page title as a parameter when pushing the
page.
Copy link
Contributor

@DanielMcInnes DanielMcInnes left a comment

Choose a reason for hiding this comment

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

lgtm. There are 11 device-type pages that have:

	title: device.name

	Device {
		id: device
		serviceUid: root.serviceUid
	}

Do you think it's worth adding a 'DevicePage' type to encapsulate this?

@blammit
Copy link
Contributor Author

blammit commented Dec 20, 2024

Do you think it's worth adding a 'DevicePage' type to encapsulate this?

I'm reluctant to if that's the only benefit. If you have a device-type page and you want to override the title, then there's no point in using DevicePage, and then do you still extend DevicePage just for the semantics? Also, once we have a base-type component, it creates restrictions, e.g. what if you have a device-type page but it needs to extend something else? I would prefer to follow the "composition over inheritance" idea here, unless we get more requirements for device-type pages that make it more necessary to have a base DevicePage.

@blammit blammit merged commit ea3d732 into main Dec 20, 2024
2 checks passed
@blammit blammit deleted the blam/device-page-titles branch December 20, 2024 02:39
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