Skip to content
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

Accordion: web example not working #2465

Closed
augustblack opened this issue Jan 26, 2018 · 15 comments · Fixed by #3065
Closed

Accordion: web example not working #2465

augustblack opened this issue Jan 26, 2018 · 15 comments · Fixed by #3065
Labels

Comments

@augustblack
Copy link

Steps to Reproduce

  1. go to: https://react.semantic-ui.com/modules/accordion#accordion-example-inverted
  2. try to edit it on the website there by clicking the '</>' button

Expected
you should be able to edit the code

Result
There are errors: unknown: Unterminated regular expression (8:12)

@augustblack augustblack changed the title fix(Accordion): your description fix(Accordion): web example not working Jan 26, 2018
@layershifter layershifter changed the title fix(Accordion): web example not working Accordion: web example not working Jan 29, 2018
@layershifter
Copy link
Member

There is problem with the from word, seems it's a problem with imports.

@brianespinosa
Copy link
Member

Not sure if this will solve the issue, but we might want to try wrapping whole strings in {'string'} brackets like this, not just the first space of line returns.

@road-cycling
Copy link
Contributor

Adding 'from' to other examples will also cause same error. from -> {'from'} not working either...Although {'fro' + 'm'} works. I took a look around but couldn't find out where it happens

@agneym
Copy link

agneym commented Feb 1, 2018

It is caused by this line:
_.get(/import[\s\S]*from.*\n([\s\S]*)/.exec(sourceCode), '[1]', '')
[\s\S]* after import removes everything till they find the last from.

@road-cycling
Copy link
Contributor

changing

    const body = _.get(/import[\s\S]*from.*\n([\s\S]*)/.exec(sourceCode), '[1]', '')
      .replace(/export\s+default\s+(?!class|function)\w+([\s\n]+)?/, '')  // remove `export default Foo` statements
      .replace(/export\s+default\s+/, '')                                 // remove `export default ...`

to

    const body = _.get(/export\sdefault\sclass[\s\S]*/.exec(sourceCode), '[0]', '')
      .replace(/export\s+default\s+(?!class|function)\w+([\s\n]+)?/, '')  // remove `export default Foo` statements
      .replace(/export\s+default\s+/, '')                                 // remove `export default ...`

fixes it given the current format

Should I make a pull request, or should I try something else?

@levithomason
Copy link
Member

levithomason commented Feb 2, 2018

@nieroda A PR would be fantastic, thanks!

The issue was that the current regex is looking for the last occurrence of from. If you paste one of the code examples into regexr.com you can try out the different regex patterns and see what they are trying to achieve.

You might want to run similar scenarios with your solution by adding the words export default class someone besides the default export and see if it breaks.

@road-cycling
Copy link
Contributor

I only saw the edit after I created the PR, let me check

@road-cycling
Copy link
Contributor

road-cycling commented Feb 2, 2018

I tried a few things, nothing is breaking. However if I add ("export default" or "export default *")inside one of the <p> tags, it does omit it from showing above, but thats due to the 2 chained replace functions I believe.

edit: only matches export class, which breaks other things. Will look for a fix

@levithomason
Copy link
Member

This is a step forward at least. Odds that we'll need to put that language in an example snippet are low. The live edit is quite the hack as it is :)

@road-cycling
Copy link
Contributor

road-cycling commented Feb 2, 2018

Sorry about that, just looked through the other examples

and saw the components were defined in these 3 ways,

class * extends Component
const * = () => (     
export default class 

Sometimes variables were declared above the class, but it would match with the top 'const' (would be an issue if var is used)

const data = [foo, bar];
class * extends Component {}

Would something like this be better?

const body = _.get(/(export\sdefault\sclass|const|class\s\S*\sextends)[\s\S]*/.exec(sourceCode), '[0]', '')
        .replace(/export\s+default\s+(?!class|function)\w+([\s\n]+)?/, '')  // remove `export default Foo` statements
        .replace(/export\s+default\s+/, '')     

Or does it make too many assumptions?

@road-cycling
Copy link
Contributor

@levithomason what do you think?

@levithomason
Copy link
Member

Apologies, I don't have the bandwidth to grok each solution 😞 Whichever one works with all existing examples and is the most robust against future possibilities, let's go with that.

@herbertpimentel
Copy link

I am not sure it is an erro but for me accordion component requires I explicity control activeIndex. is that expected behaviour ?!

Edit Semantic UI React Accordion

@stale
Copy link

stale bot commented Jul 16, 2018

There has been no activity in this thread for 90 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 90 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

@stale stale bot added the stale label Jul 16, 2018
@crashuniverse
Copy link
Contributor

I looked at the Accordion - inverted variation today and it seems to work fine with its edit code feature. The problem no longer exists. Can one of us verify it and close if this is the case?

New link for Accordion - inverted variation: http://react.semantic-ui.com/modules/accordion/#variations-inverted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants