Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Adding styling to show differences between selected states #169

Merged
merged 3 commits into from
Feb 7, 2019

Conversation

hrigner
Copy link

@hrigner hrigner commented Feb 4, 2019

Closes #168

@hrigner hrigner requested a review from peol February 4, 2019 19:35
@netlify
Copy link

netlify bot commented Feb 4, 2019

Deploy preview for catwalk-qlikcore ready!

Built with commit 120c0bc

https://deploy-preview-169--catwalk-qlikcore.netlify.com

@hrigner
Copy link
Author

hrigner commented Feb 4, 2019

I don't know if this approach is obvious.. @peol, please have a look 😄

Copy link
Contributor

@peol peol left a comment

Choose a reason for hiding this comment

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

LGTM, but I don't have an app I can easily reproduce it on so perhaps @sublibra can look at it too

@@ -101,10 +101,10 @@ function useSelections(model, layout, selfRef) {
const onRowClick = async ({ rowData }) => {
if (rowData) {
const rowDataToModify = rowData;
if (rowData[0].qState !== 'S') {
if (rowData[0].qState !== 'S' && rowData[0].qState !== 'XS') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this case is 100% correct, might be a scenario where mapping it to 'S' is wrong, but I cannot test now and edge cases here are fine by me.

@hrigner
Copy link
Author

hrigner commented Feb 6, 2019

So, new color scheme for selected but excluded. Please have a re-look :)

@sublibra
Copy link
Contributor

sublibra commented Feb 6, 2019

It matches more closely to QlikView and is consistent! I like it!

In the future we might need to investigate the introduction of a third selection mechanism (QlikView has ctrl-click, Qlik Sense green mark, catwalk click outside the component). But this shouldn't be handled in the PR.

@sublibra sublibra self-requested a review February 6, 2019 20:26
@hrigner hrigner merged commit 68473bd into master Feb 7, 2019
@axelssonHakan axelssonHakan deleted the selected-excluded branch February 8, 2019 09:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants