mistercrunch commented on PR #34526: URL: https://github.com/apache/superset/pull/34526#issuecomment-3177468330
> I think it would be better to hide the Top N feature and it's controls for now instead of displaying the 'Not implemented yet' tag with controls that won't work. Being behind a feature flag and with a `beta` feels like it's ok to make things a bit more of a construction site, showing things not fully all done and a bit of scaffolding. Frankly on TopN it's probably easier to push forward and build the feature than removing/re-adding the controls. > What do you think of moving the Show row labels and Show column headers to the Columns/Rows containers and make them appear only when there are dimension/metric values selected? Displaying an empty header when there's no value selected is kind of useless. This could also simplify the UI by removing the Matrix section. Seems very reasonable, better than what it is now, though `Matrix` seemed like a good placeholder that eventually might have more things, but until it does, fine with shuffling controls around. > Should we also match the style of chart titles with other charts? They don't have a background color and no bottom border. cc @kasiazjc Yeah did some serious thinking here and punted on getting ambitious. Ideally the chart cards in dashboards would use similar/same headers and action bar as what we find here, including things like exporting CSVs and all. Could be a longer term goal to have dashboards and "recursive charts" use the same cards and headers. If anything here and now in this PR, I'd keep it on doing a purely cosmetic alignment, though it's tempting to try and emulate the dashboard chart card and it gets complicated quickly. Maybe the fact that it looks different is a somewhat good thing as it IS different, users shouldn't expect all dashboard-card features to work in this context, at least not yet... > I think it's safe to remove the Beta tag as we don't do that for other features? I think I like it there, maybe until the feature flag is removed and this becomes a core feature. Would love for admins to switch it on and users to try it, but also be warned about the fact it's new and stabilizing. Also pops a little and makes the new tab discoverable ... I don't feel super strongly about it, but I kind of like it there > It looks like cross-filtering is not working as the charts are not being affected. I also noticed that left-clicking a data point is not adding a cross-filtering. Only the right-click option seems to be available. Ah, yeah it doesn't work within itself, but should work with other charts (send and receive should work). Might need to think a bit deeper and disable some options here as they might be confusing? Drill through works nicely though! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
