Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improve dashboard title #228
Improve dashboard title #228
Changes from 4 commits
4def8ed
cfc3cf7
e11f3b6
20dcd1a
5cef34a
c00fe4e
7f981fb
b5ea319
708bb8e
638f0b0
403097a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably just left this in for demo purposes, but I would remove it if this is close to being merged. It's not optimal in terms of design if there are controls/navigation on the page, as we still have this open discussion of where this might go.
Looking at this, I prefer the upper header container we discussed the other day even more 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I left it here so that we would hopefully apply the same change to vizro.mckinsey.com next time we update the demo, but let me remove it if we're not yet happy about the visual design.
The reason I think it would be nice to include sooner rather than later is because every time someone shares a link to vizro.mckinsey.com the social media preview will have a slightly strange label (like just "Homepage") - it would be nicer if it instead said "Vizro demo: Homepage" or similar, to make it clear what the link being shared actually is.
If we don't work out the page layout/visual design soon then possible alternatives might be:
_make_page_layout
for nowSo I'll revert the change here and leave it up to you to decide out what's the best solution here depending on how long the reworking of the page layout takes 🙂 It's not a huge thing but I don't think we should wait too long (say > 1 month) to get an improved shareable link to our demo app just because we're blocked on where to put the title on screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we remove the title from the dashboard layout altogether from _make_page_layout for now
I think this would be a breaking change right? Or are we still fine saying that any visual changes are not considered breaking changes? 😄 I would be fine with this one to be honest - if people reach out, then we at least know people have been using a dashboard title and need it 😅
we remove the title from the dashboard layout just by customising our demo app
As a quick-fix, this one seems safer. The demo app would be a bit out of sync for a while with the example app here, but I think it's fine as we manage it.
What do you prefer? 1) I could embed in my current PR on layout arrangements and 2) I can create in another PR in the demo-app repository
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted in 5cef34a.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't consider removing the title from the layout a breaking change since it's just something visual and no one's code will break in any way. Admittedly it's kind of a drastic one rather than just tweaking styling, but I'm completely fine with it and, like you say, if people ask where their title went then it's a good signal they want it there.
Which solution we go for depends to me on how close we are to resolving the question of where the title goes and how upsetting people find the current placement:
Personally I'm completely fine with the current visual appearance, which is why I just went for option 3 to begin with in this PR. But you have a better idea of how upsetting people with more design sensibilities find that and where future changes to layout might take us here, so completely up to you which option to choose 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes a lot of sense!
I think the position of the dashboard title will likely change given that it's taking too much horizontal space if there are no controls/navigations. Given that J. is also out, I'll go for 2) as I think it's the one he would feel less reluctant to implement 😄
I add it to my tickets and will update the demo accordingly after we've done release 0.1.8 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thank you! Sounds good to me.
I didn't actually realise how weird it looks like when there's no controls/navigation until I just looked again now. You're right that doesn't look good at all, so I take back my statement above that option 3 was ok by me. Just in case you thought I had no design sensibilities at all 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P.S. hopefully the demo update would be as simple as
dash.layout["dashboard_title_outer"].hidden = True
or similar rather than needing any customisation of_make_page_layout
with a customDashboard
model.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one line is the only functional change in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if people do want to overwrite it, they would overwrite the pre-build of the model here?
I was actually wondering how this works compared to the
dash.app.title
. If you set thedash.app.title
, I noticed that it appears for a second and then it gets overwritten by anything defined indash.register_page
.This might be worth mentioning in the docs if people try to overwrite it via the
dash.app.title
. This wouldn't actually work, because in the end it gets overwritten by the code here again. But I think this is also how it works in a pure Dash app?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed how it works in a pure Dash app also:
Dash(title="X")
corresponds to the propertydash.app.title
and sets the initial value of<title>X</title>
dash.page_registry[page.id]["title"]
title=X
has no effect on anythingSo for Vizro users where Dash pages is always used, the
title
argument basically has no effect. @AnnMarieW please do correct me if I've missed something here.We have possibly added to the confusion here because we have our own
Dashboard.title
property, and it's not obvious to a user what effect that has on anything. I considered before whether we should dodash.app.title = dashboard.title
inVizro.build()
but decided it was basically pointless because, like you say, it just gets overwritten straight away. I think it would be a very small improvement though, since that way you wouldn't see the "Dash" title at all, so let me add it in here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change made in c00fe4e.