Skip to content

Conversation

@megoth
Copy link
Contributor

@megoth megoth commented Aug 28, 2019

By providing option onClose, tabs are extended with an extra, unstyled tab that contains a cancelButton. Once clicked it will call the given onClose-method.

This is needed for providing the cancelButton for the global dashboard.

Part of fix for SolidOS/solid-panes#154

@megoth megoth requested a review from timbl August 28, 2019 17:35
megoth referenced this pull request in SolidOS/solid-panes Aug 28, 2019
This presents a cancelButton next to the tabs.

This relies on changes in https://github.com/solid/solid-ui/pull/110
Copy link
Contributor

@timbl timbl left a comment

Choose a reason for hiding this comment

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

Please test with updated version of t .../tests/

megoth added 2 commits August 29, 2019 19:13
By providing option onClose, tabs are extended with an extra, unstyled tab that contains a cancelButton. Once clicked it will call the given onClose-method.

This is needed for providing the cancelButton for the global dashboard.

Part of fix for SolidOS/solid-panes#154
@megoth megoth force-pushed the add-cancel-button-to-tabs branch from 0db7d0c to c737fe1 Compare August 29, 2019 17:13
@megoth
Copy link
Contributor Author

megoth commented Aug 29, 2019

Rebased onto master, and tested with the test-files.

I did check that the original tests went through, but the test-setup is waaaaaay to slow that I want to use it to test this new part as well.

@megoth
Copy link
Contributor Author

megoth commented Aug 29, 2019

@timbl should I add a new tests, or is it ok to just check the existing tests?

@megoth megoth changed the title Adds feature to add cancelButton to end of tabs Adds optional cancelButton to end of tabs Aug 30, 2019
@timbl
Copy link
Contributor

timbl commented Aug 30, 2019

The existing tests build a tabs widget and morph its state, with each orientation. The idea is you can for example check the positioning CSS works. I suggest just adding the option for the X box to the test. Add a dummy onClose function to the core of the test loop. (Didn't I do that?) so that you can see that the positioning CSS works for that too in each orientation. I don't think you need to check that it works without the X button again.
Sorry I thought that was clear

@timbl
Copy link
Contributor

timbl commented Aug 30, 2019

Did you run the tests on #110 with the X button included?
onClose() included?
Did the x pup up in a reasonable place for each orientation?
If so I am fine to merge

@megoth
Copy link
Contributor Author

megoth commented Sep 2, 2019

Ok, I've made it kinda work - but we need to have a discussion on the tests; They are not easy to read, and since there are no documentation I have to go a lot back and forth between the HTML, Turtle and JavaScript to make sense of it. I'm not a fan of these kinds of custom test setups, and we should investigate the possibility of making use of snapshots, like Snapshot Testing in Jest.

@megoth megoth merged commit d5962a8 into master Sep 2, 2019
@megoth megoth deleted the add-cancel-button-to-tabs branch September 2, 2019 10:28
megoth referenced this pull request in SolidOS/solid-panes Sep 2, 2019
This presents a cancelButton next to the tabs.

This relies on changes in https://github.com/solid/solid-ui/pull/110
@brownhoward brownhoward added this to the Data Browser Release 2 milestone Oct 28, 2019
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.

4 participants