-
Notifications
You must be signed in to change notification settings - Fork 2
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
Provide descriptions against FCDOs and FCDAs in dataset view and picker (closes #6) #7
Provide descriptions against FCDOs and FCDAs in dataset view and picker (closes #6) #7
Conversation
1f2de12
to
4f376e5
Compare
hi @JakobVogelsang WDYT about this? Can we discuss a little if this is an acceptable approach and practical within our current UI? |
4f376e5
to
afff9f8
Compare
With OpenEnergyTools/filterable-lists#6 some screenshots and an updated PR. I've also made the full path be in the headline rather than the supporting text which TBH I think is easier to parse. With smallish descriptions I don't think the whitespace is bothersome: When the descriptions mean the list item is very full it becomes a little hard to read. Just barely acceptable in the dataset view? I believe the list items are now always in line with the action items, but the list item height cannot vary with the supporting text so there is no more |
@JakobVogelsang as part of looking at OpenEnergyTools/filterable-lists#6 I would also be grateful if you could look at whether the changes here would be acceptable or heading in the right direction. |
4370e08
to
7a96e70
Compare
This evening made some improvements with help from @ca-d to improve theming and distinguish the headline from the supporting text. Now looks more like: Much better visual distinction when descriptions are large: |
Not exactly finished, but ready for an initial review. |
I will fix some broken tests and add new tests now, I feel this is close. |
I have done what I can to fix tests and make them repeatable but I have not been entirely successful. I have restricted tests to Chrome (there are way too many to visually check on multiple browsers given the current level of repeatability). This is a somewhat opinionated approach to descriptions - use the desc field preferentially, and DAI[d] otherwise and don't use DTTs but I think this is reasonable and consistent with what I've seen implememented. It's certainly a starting point. Obviously it needs a release/update of @JakobVogelsang at an appropriate time I would be delighted to receive a review and happy to make improvements. I'm keen to get this over the line. I've done quite a few "drive-by" improvements/fixes (including fixing some earlier tests I broke). Nonetheless the whole visual tests remain quite unstable (but better than before). This has ended up being a larger PR than I expected... |
Hi @danyill , this plugin is the next one to be scoped. As a first job I have fixed some security alerts and fixed tests. They are now running on chromium only and should be reliable. As a next step I will scope the plugin. Then I need to ask you to rebase again. I am sorry for giving so much extra work. My time is limited atm and this is the best spped I can do. |
No problem - I am grateful for your efforts and happy to rebase. I will then need to add the polyfill to our distribution and may need a little help. |
This long-winded PR I will close in favour of #34 as it had many merge conflicts with the scoped component work. |
Closes #6. See #6 for some screenshots of the current state to allow discussion of how to meet this goal.
We can also decide if this is an appropriate way to show descriptions.
I've decided:
<DAI name="d">
,VSD
, nsdocs or others...I finished the DA picker to illustrate the change but would need to do more:
As part of a drive by I also:
Happy to split these out as required.